Skip to content

Fix Product Flavour builds#5580

Closed
jaggs6 wants to merge 1 commit intofacebook:masterfrom
jaggs6:master
Closed

Fix Product Flavour builds#5580
jaggs6 wants to merge 1 commit intofacebook:masterfrom
jaggs6:master

Conversation

@jaggs6
Copy link
Contributor

@jaggs6 jaggs6 commented Jan 27, 2016

  • Capitalise productFlavorName for target path
  • Capitalise buildType in sourceName when flavour exists
  • Fix the path the assets are added.
  • Backward compatibility with bundleIn(buildTypeName) when flavours are added

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @AndrewJack, @foghina and @mkonicek to be potential reviewers.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

@jaggs6 updated the pull request.

@AndrewJack AndrewJack mentioned this pull request Jan 27, 2016
@facebook-github-bot
Copy link
Contributor

@jaggs6 updated the pull request.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@jaggs6 updated the pull request.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 27, 2016
@AndrewJack
Copy link
Contributor

Looks good 👍

Copy link

Choose a reason for hiding this comment

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

Won't this become the gradle task assembleflavorDebug when we want assembleFlavorDebug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, Because it will use the targetName variable everywhere and not the sourceName one.

Copy link
Contributor

Choose a reason for hiding this comment

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

It uses targetName to generate the task name:

// Bundle task name for variant
def bundleJsAndAssetsTaskName = "bundle${targetName}JsAndAssets"

Copy link

Choose a reason for hiding this comment

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

Ok! What about

def targetName = "${productFlavorName.capitalize()}${buildTypeName.capitalize()}"
def targetPath = "${buildTypeName}"
if (productFlavorName) {
  targetPath = "${productFlavorName}/${buildTypeName}"
}

to make it more readable and if we're not using sourceName anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good. Let me test it.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 yep thats better. Are we going to use this PR or #5579 ?

@facebook-github-bot
Copy link
Contributor

@jaggs6 updated the pull request.

@jaggs6 jaggs6 changed the title Fix Product flavour builds Fix Product Flavour builds Jan 27, 2016
@jaggs6
Copy link
Contributor Author

jaggs6 commented Jan 28, 2016

@foghina and @mkonicek what do you guys think?

@mkonicek
Copy link
Contributor

Looks good. How did you test this? Can you show some short examples of commands you ran to demo it works? It's just thing we do at fb that every commit should have so a called "Test Plan".

@jaggs6
Copy link
Contributor Author

jaggs6 commented Jan 29, 2016

@mkonicek thanks for looking into it. Here is how to test each change.

  1. and 2) just run ./gradlew tasks in the android folder (you will see the new task names capitalises properly) you will have to put print $targetName below my assignment change to see the diff.

  2. (without my change) just make a flavour in the app/build.gradle and then run the app for that flavour. make sure you are running the release verison of that flavour. You will notice that it requires the react native server running even for a flavoured release build. This is because the wrong asset folder was being bundled. My change fixes that.

  3. add bundleInSnapshot true in the config on top of the app/build.gradle. and add a build type snapshot. you will see that it maintains the bundle option to true for all the flavours in that build type.

@jaggs6
Copy link
Contributor Author

jaggs6 commented Jan 31, 2016

@mkonicek does the above answer your question?

@mkonicek
Copy link
Contributor

mkonicek commented Feb 8, 2016

Cool, thanks for the test plan. Let's merge it. Sorry for the delay, just came back from vacation.

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1667939000161821/int_phab to review.

@ghost ghost closed this in 1cf605d Feb 8, 2016
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
- Capitalise productFlavorName for target path
- Capitalise buildType in sourceName when flavour exists
- Fix the path the assets are added.
- Backward compatibility with bundleIn(buildTypeName) when flavours are added
Closes facebook/react-native#5580

Reviewed By: svcscm

Differential Revision: D2911735

Pulled By: mkonicek

fb-gh-sync-id: 6fb391a12ee27ee2a503961d8779a85d31cf5367
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants