Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more config parameters for android_build #34

Conversation

compojoom
Copy link
Contributor

Summary

Add more parameters to the android_build command. This allows us to be more explicit with our build variants and it terms will save minutes on circleCI by only specifying the build variantes that we really needs

fixes #33

This is a breaking change!
We need to specify the following parameters

  max_workers:
    description: The number of workers to use for a build
    type: integer
    default: 2
  assemble_build_type:
    description: The build variant to build. This is normally either "assembleDebug" or "assembleRelease" but you may have custom build variants that you can specify here.
    type: string
    default: "assembleDebug"
  assemble_test_type:
    description: The test build variant to build. This is normally "assembleAndroidTest" but you may have custom build variants that you can specify here.
    type: string
    default: "assembleAndroidTest"
  test_build_type:
    description: The test build type to build. This is normally "debug" or "release".
    type: string
    default: "debug"

where previously we only had build_type.

If we don't specify any parameters then the config should have the old behavior.

I also updated the android_build job with the new syntax

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)

Add more parameters to the android_build command. This allows us to be more explicit with our build variants and it terms will save minutes on circleCI by only specifying the build variantes that we really needs

fixes react-native-community#33
Copy link
Member

@matt-oakes matt-oakes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Thanks for sending this in. Apologies for the delay in reviewing it.

I'm wondering if this could be achieved without a breaking change. To do this, you could add a new flavour parameter, which defaults to an empty string, and then using it in the command like this:

"cd <<parameters.project_path>> && chmod +x gradlew && ./gradlew --build-cache --max-workers 2 --continue assemble<<parameters.flavour>><<parameters.build_type>> assemble<<parameters.flavour>>AndroidTest -DtestBuildType=<<parameters.build_type>> --stacktrace"

You should then be able to achieve what you need and only build a single set of APKs, but would introducing a breaking change in the Orb. Would that work for you?

@compojoom
Copy link
Contributor Author

Hm, that is not a bad idea. It could work for my case. I was however thinking further on this and I don't really like my PR in the first place. After submitting it I started thinking that it would be much better if we had just gradlew_params and the default value could be

--build-cache --max-workers 2 --continue assembleRelease ...

What we have right now. And then we could override it with whatever we want. I know this is again a breaking change, but it allows better flexibility with less params. What do you think?

@matt-oakes
Copy link
Member

The commands in this Orb are meant to cover the basic use cases, with some customisation options so it's flexible enough to use with most apps. The idea isn't to be infinitly flexible.

If a command doesn't offer the flexibility that someone needs, they can always just write a custom step and ignore the library one.

@compojoom
Copy link
Contributor Author

@matt-oakes that's a fair point and I think that by changing the above it will work with "most apps" as you said. I know that I can copy it and modify it, but I still think that if we could provide something that works for "most apps" it will improve developer productivity. It's much better to configure a line, than to copy 50 just because one can't configure a line...

I think that gradlew_params would be the most versatile approach and will cover most use cases.

It's up to you. If you don't want this, I'll close the PR.

@compojoom
Copy link
Contributor Author

@matt-oakes I just realized that this PR status is "change requested". So what do you want me to do?
add flavor or default gradlew_params or nothing at all :)

I personally am in favor of gradlew_params. But I feel that flavor is not making it better. Tomorrow someone will need yet another param and when do we say enough is enough? On the other hand gradlew_params let's users do their thing...

@matt-oakes
Copy link
Member

I see what you mean about making it so people aren't required to break out from the command.

Could we do both for the best flexibility?

If you added in the flavour with the default being empty, then we can make this a none breaking change.

You could then also include a gradlew_params parameter if people want to customise it further.

Would that work or is that just complicating things even more?

@compojoom
Copy link
Contributor Author

You mean like <<parameters.additional_gradlew_params>>:

"cd <<parameters.project_path>> && chmod +x gradlew && ./gradlew --build-cache --max-workers 2 --continue assemble<<parameters.flavour>><<parameters.build_type>> assemble<<parameters.flavour>>AndroidTest -DtestBuildType=<<parameters.build_type>> --stacktrace <<parameters.additional_gradlew_params>>"

or if gradlew_params set - ignore all other params and just use gradlew_params?

@matt-oakes
Copy link
Member

I think additional makes more sense. If they want to override the whole thing then they may as well just not use the command and write their own.

@compojoom
Copy link
Contributor Author

compojoom commented Apr 24, 2020

@matt-oakes just started implementing this and realized that it won't work the way you are suggesting.
The command we have to execute is:

./gradlew assembleDevelopRelease assembleDevelopReleaseAndroidTest -DtestBuildType=release 

just adding a flavor variable won't cut it. The command right now is:

command: "cd <<parameters.project_path>> && chmod +x gradlew && ./gradlew --build-cache --max-workers 2 --continue assemble<<parameters.build_type>> assembleAndroidTest -DtestBuildType=<<parameters.build_type>> --stacktrace"

if I add flavor=Develop then I would end with assembleDevelopRlease which is fine. But the AndroidTest would be assembleDevelopAndroidTest - which won't work...

I'm giving up on this. I'll copy the whole android_build job in our project and just modify the build step there.

@compojoom compojoom closed this Aug 7, 2020
@chakrihacker
Copy link
Member

@matt-oakes is it necessary to run Gradle task assembleAndroidTest?

I would like to help and fix this issue as we use product flavors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Build Android APK step
3 participants