-
Notifications
You must be signed in to change notification settings - Fork 935
fix: custom root or calling Gradle outside of project directory #1057
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
Conversation
| * Sometimes Gradle can be called outside of JavaScript hierarchy. Detect the directory | ||
| * where build files of an active project are located. | ||
| */ | ||
| def projectRoot = rootProject.projectDir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context: rootProject is what you give a name in settings.gradle - it's the umbrella in case you have different smaller sub-projects
For autolinking and CLI, it doesn't matter - we always want the main project. But you will notice that applyNativeModulesBuildGradle expects project argument to handle such case.
|
After this PR, I'll provide a similar workaround for iOS. Just in case somebody ends up calling |
thymikee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! I'm not sure why I couldn't find rootProject.projectDir variable
|
Added I would say let's merge it and it if ends up being flaky, I am volunteering to fix it ASAP. Don't want to do premature optimisation tho. |
Co-Authored-By: Michał Pierzchała <[email protected]>
|
|
||
| // Execute `gradle` with `-p` flag and `cwd` outside of project hierarchy | ||
| const {stdout} = spawnScript(gradleWrapper, ['-p', androidProjectRoot], { | ||
| cwd: '/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set cwd here? I'd expect a default value would be sufficient – this way we could make the options argument of spawnScript optional with {} as default value, as it used to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truth is - we never want to rely on default value in our tests and that's why I decided to go with explicit cwd.
In this case, I just want to make sure and be 100% explicit that cwd is somewhere else than the project - for example, at the root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truth is - we never want to rely on default value in our tests and that's why I decided to go with explicit cwd.
The reason why I believe it's better to require cwd is simple - cwd defaults to the location where the React Native CLI source files are located (or where the tests are being run from).
Conceptually, I think of e2e tests as something run in a totally separate environment that is close to what users are doing in real life. I feel like setting cwd in spawnScript (which I consider advanced use case, where runCLI is not enough) is just a way to tell "watch out, you're going into advanced territory - think about what you are doing".
TL;DR:
Fixes #995
Fixes #405
Fixes #793
Fixes #804
Improves #824 that was reverted via #852 because it caused regressions (see case 2)
Context:
React Native CLI is flexible and makes no assumptions where and how you structure your project folders (at least that's our ideal scenario). However, Gradle and CocoaPods need to call the React Native CLI to read the configuration, list of all dependencies to link and get some relative paths. In order to resolve where the React Native CLI currently is, Gradle and CocoaPods ask
nodeand itsrequire.resolvestrategy to locate wherereact-nativeis located.In 99% cases, Gradle is called from the project root directory or a folder within the project root, such as
android(when you run./android/gradlew). That makesrequire.resolveto work correctly, by locating thereact-nativein the closestnode_modulestraversing up the tree.However, there's that 1% of use cases when this is not true. For example, Gradle can be sometimes given an absolute path to a project to build (via
-pflag), which will result in acwdthat is different than the project root and outside of JavaScript hierarchy. This is very common for tools such asfastlaneorapp centerand happens more often when doing a release of the apk rather than installing on a simulator.Case 1
For example, calling:
<path_to_gradle>/.gradlew -p Repositories/HelloWorld/androidfrom/home/grabbouwill result in the #995 error, becausecwdis/home/grabbouand there's no/home/grabbou/node_modules/react-nativeto find.Case 2
The previous
jsAppDirworkaround contributed by @thymikee wasn't optimal either, as it was inferringrootof a project from the directory wherenode_modulesare. In a monorepo, the dependency will be hoisted, which will result in therootbeing set incorrectly.At the time of reverting this feature in #852, we didn't know why it was causing us troubles. However, recently, thanks to additional reproductions provided by the community, we were able to understand why that particular issue was happening.
For example, calling
<path_to_gradle>/.gradlew -p Repositories/HelloWorld/packages/app-a/androidwith the aforementioned workaround would setrootto/users/grabbou/Repositories/HelloWorldbecause React Native dependency has been hoisted to/users/grabbou/Repositories/HelloWorld/node_modules/. In reality, we want the actual/users/grabbou/Repositories/HelloWorld/packages/app-a.Solution:
The solution turned out to be simple and as I initially expected, was a matter of finding the right Gradle variable to read from.
Reading
rootProject.projectDirand using that as arootalways points out correctly to the folder that is either inferred automatically or set via-pflag.Test Plan:
Run
gradlewith-pflag outside of JavaScript hierarchy.