Skip to content

Commit

Permalink
Update link command for Android project (#20853)
Browse files Browse the repository at this point in the history
Summary:
Motivation:
--------------
PR #20767 bumped the version of the Android Gradle Plugin to v3 which uses the newer Gradle dependency configurations `implementation` and `api` which make `compile` obsolete.

While the PR updated the template Gradle configuration, it did not cover the `link` command which will still link native modules using `compile` resulting in a warning message beeing displayed during an app build.

Since `compile` will be eventually removed by Gradle, this commit updates the `link` command to attach native modules using `implementation`.
Pull Request resolved: #20853

Differential Revision: D9733888

Pulled By: hramos

fbshipit-source-id: 22853480d7ba7be65e3387effda2fd6c72b6906a
  • Loading branch information
matei-radu authored and facebook-github-bot committed Sep 8, 2018
1 parent db9b468 commit 4dfdec9
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 14 deletions.
6 changes: 3 additions & 3 deletions local-cli/link/__fixtures__/android/build.gradle
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
dependencies {
compile fileTree(dir: "libs", include: ["*.jar"])
compile "com.android.support:appcompat-v7:27.1.1"
compile "com.facebook.react:react-native:+"
implementation fileTree(dir: "libs", include: ["*.jar"])
implementation "com.android.support:appcompat-v7:27.1.1"
implementation "com.facebook.react:react-native:+"
}
10 changes: 5 additions & 5 deletions local-cli/link/__fixtures__/android/patchedBuild.gradle
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
dependencies {
compile project(':test')
compile(project(':test2')) {
implementation project(':test')
implementation(project(':test2')) {
exclude(group: 'org.unwanted', module: 'test10')
}
compile fileTree(dir: "libs", include: ["*.jar"])
compile "com.android.support:appcompat-v7:27.1.1"
compile "com.facebook.react:react-native:+"
implementation fileTree(dir: "libs", include: ["*.jar"])
implementation "com.android.support:appcompat-v7:27.1.1"
implementation "com.facebook.react:react-native:+"
}
10 changes: 6 additions & 4 deletions local-cli/link/__tests__/android/makeBuildPatch.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,27 @@ describe('makeBuildPatch', () => {

it('should make a correct patch', () => {
const {patch} = makeBuildPatch(name);
expect(patch).toBe(` compile project(':${name}')\n`);
expect(patch).toBe(` implementation project(':${name}')\n`);
});

it('should make a correct install check pattern', () => {
const {installPattern} = makeBuildPatch(name);
const match = `/\\s{4}(compile)(\\(|\\s)(project)\\(\\':${name}\\'\\)(\\)|\\s)/`;
const match = `/\\s{4}(implementation)(\\(|\\s)(project)\\(\\':${name}\\'\\)(\\)|\\s)/`;
expect(installPattern.toString()).toBe(match);
});
});

describe('makeBuildPatchWithScopedPackage', () => {
it('should make a correct patch', () => {
const {patch} = makeBuildPatch(scopedName);
expect(patch).toBe(` compile project(':${normalizedScopedName}')\n`);
expect(patch).toBe(
` implementation project(':${normalizedScopedName}')\n`,
);
});

it('should make a correct install check pattern', () => {
const {installPattern} = makeBuildPatch(scopedName);
const match = `/\\s{4}(compile)(\\(|\\s)(project)\\(\\':${normalizedScopedName}\\'\\)(\\)|\\s)/`;
const match = `/\\s{4}(implementation)(\\(|\\s)(project)\\(\\':${normalizedScopedName}\\'\\)(\\)|\\s)/`;
expect(installPattern.toString()).toBe(match);
});
});
4 changes: 2 additions & 2 deletions local-cli/link/android/patches/makeBuildPatch.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ const normalizeProjectName = require('./normalizeProjectName');
module.exports = function makeBuildPatch(name) {
const normalizedProjectName = normalizeProjectName(name);
const installPattern = new RegExp(
`\\s{4}(compile)(\\(|\\s)(project)\\(\\\':${normalizedProjectName}\\\'\\)(\\)|\\s)`,
`\\s{4}(implementation)(\\(|\\s)(project)\\(\\\':${normalizedProjectName}\\\'\\)(\\)|\\s)`,
);

return {
installPattern,
pattern: /[^ \t]dependencies {(\r\n|\n)/,
patch: ` compile project(':${normalizedProjectName}')\n`,
patch: ` implementation project(':${normalizedProjectName}')\n`,
};
};

11 comments on commit 4dfdec9

@grabbou
Copy link
Contributor

Choose a reason for hiding this comment

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

I have one comment regarding this PR:
1/ Did it pass the test suite?
2/ That makes it incompatible with older modules and will make them impossible to link, right? I believe we should support detecting both compile and implementation when unlinking so that it keeps working for everyone. At the same time, compile might be already there in the codebase and we want to be aware of it when looking for place to start adding new dependencies.

@grabbou
Copy link
Contributor

Choose a reason for hiding this comment

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

CC: @Kureev as you originally authored that part, is there anything in particular we should be aware of when it comes to dealing with this change?

@Titozzz
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like you should be aware of what this tool is doing when you use it, and that writing this change as a breaking change that you need to replace compile with implem manually during 0.57 upgrade seems fine to me.

@matei-radu
Copy link
Contributor Author

@matei-radu matei-radu commented on 4dfdec9 Sep 10, 2018

Choose a reason for hiding this comment

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

Replying from my phone so bear with me.

@grabbou

  1. Test suite did pass, although not on CI since at the time of the PR some steps of the pipeline where broken. However, tests concerning link passed.

  2. Existing modules are not affected and will be linked correctly, but you're right about unlinking! I did not think about existing projects upgrading to 0.57.

I agree we should make unlink aware of both compile and implementation. I wouldn't mind working on these changes if it would be OK. We should also mention it as a breaking change like @Titozzz suggested.

EDIT: if we actually fix this in time for release then we wouldn't have a breaking change anymore, but we should still inform developers (of both apps and modules) to update their Gradle configs in the next months.

EDIT 2: I found a working solution. I will proceed with adding some tests and making a PR.

@fungilation
Copy link

Choose a reason for hiding this comment

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

@Titozzz I see a half dozen packages I use with breakage in themselves for using compile, so "awareness" definitely needed to get all the RN packages out there updated too.

@Titozzz
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean breakage ? Compile will still work for now. The only issue here would be unlinking packages previously linked with compile. Or linking again old packages that are not using implementation.

To me it just depends if link is supposed to be a tool or a magic wand 😁

@matei-radu
Copy link
Contributor Author

Choose a reason for hiding this comment

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

About "linking again old packages that are not using implementation", do we really want that option? Using implementation with old packages works just as fine as before.

@fungilation
Copy link

Choose a reason for hiding this comment

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

Rephrase: not breakage yet, only warnings (from within outdated packages). Until compile gets deprecated.

And yes, implementation seems to be a complete replacement for compile from what I read of Google's docs that shouldn't break existing packages. I changed a dozen package deps in my app's build.gradle from compile to implementation, and my app builds and run fine. (android emulator on windows)

@Titozzz
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fungilation Basically, api = compile (literally) and implementation is something different, when using it you say that you don't really care about what inside but you need it. It will speed up compile time and allow for more optimizations I think. If you can't decide between api and implementation, try implementation, if it does not work, then use api.

@matt-block I was not very clear, sorry, let me rephrase.

With theses change and no compatibility with compile being done, there would be 2 issues:

  1. It would become impossible to unlink previously 'compile' packages
  2. It would relink any 'compile' package when running react-native link since it would not detect implementation

Possible solutions

  1. Easy to do, just also detect compile and remove it
  2. We need to decide what would happen, ideally we would need that every compile gets changed to implementation, (I'm not sure if api instead would be needed but that would also be something to take into account)

@matei-radu
Copy link
Contributor Author

@matei-radu matei-radu commented on 4dfdec9 Sep 11, 2018

Choose a reason for hiding this comment

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

@Titozzz thanks for clarifying 😄

In #21043 I address what you pointed out except for the part of updating existing compile dependencies to implementation.

I'm in favour of updating existing compiles when trying to link them again. If we agree with that I can add it to the PR. If not, we have to inform users to manually update their build.gradle file.

@Titozzz
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you should try to update, but that's more work on you ^^

Please sign in to comment.