Add support for @classpath_abi macro #1780

raviagarwal7 · 2018-02-21T21:45:13Z

This is similar to @classpath macro but gives the corresponding
class-abi jar for the classpath entries.

This is useful for genrules which only depends on the dependencies abi’s.
For eg: android lint where it need not run the lint checks again if
the dependent’s abi doesn’t change.

Usage: $(@classpath_abi :<target_path>)

styurin

CI build is failing.

Could you provide more context about why you need that and update the documentation?

@classpath

This is similar to @classpath macro but gives the corresponding class-abi jar for the classpath entries. This is useful for genrules which only depends on the dependencies abi’s. For eg: android lint where it need not run the lint checks again if the dependent’s abi doesn’t change. Usage: `$(@classpath_abi :<target_path>)`

facebook-github-bot · 2018-02-21T23:23:33Z

@raviagarwal7 has updated the pull request.

raviagarwal7 · 2018-02-21T23:25:17Z

Added more context in the description and also added to the macros soy.
Have fixed the build errors as well.

styurin · 2018-02-21T23:29:47Z

src/com/facebook/buck/jvm/core/HasJavaAbi.java

@@ -43,18 +43,18 @@
 Flavor SOURCE_ONLY_ABI_FLAVOR = InternalFlavor.of("source-only-abi");
 Flavor VERIFIED_SOURCE_ABI_FLAVOR = InternalFlavor.of("verified-source-abi");

- static BuildTarget getClassAbiJar(BuildTarget libraryTarget) {


Any changes in this file? Why do you need to move this method?

just moved the method to be in accordance with others, but will revert back

styurin · 2018-02-21T23:31:50Z

src/com/facebook/buck/rules/macros/ClasspathMacroExpander.java

@@ -85,7 +85,7 @@ protected Arg expand(SourcePathResolver resolver, ClasspathMacro ignored, BuildR
 // The # characters that might be present in classpaths due to flavoring would be read as
 // comments. As a simple workaround, we quote the entire classpath.
 private static class ClassPathWriteToFileArg extends WriteToFileArg {
- public ClassPathWriteToFileArg(BuildTarget target, String prefix, Arg delegate) {


Changes in this file look unrelated, could you revert them?

Intellij was showing suggestions that this is not needed, will revert back.

raviagarwal7 · 2018-02-21T23:39:43Z

src/com/facebook/buck/rules/macros/ClasspathAbiMacroExpander.java

+
+ ImmutableList<SourcePath> jarPaths =
+ getHasClasspathEntries(inputRule)
+ .getTransitiveClasspathDeps()


getTransitiveClasspathDeps returns both #aar_prebuilt_jar,class-abi and #class-abi flavours. I verified that both the output jars are same, just on different paths.

//cache:com.support-compat-27.0.2.aar#aar_prebuilt_jar,class-abi buck-out/gen/cache/com.support-27.0.2.aar#aar_prebuilt_jar,class-abi/com.support-27.0.2.aar-abi.jar //cache:com.support-compat-27.0.2.aar#class-abi buck-out/gen/cache/com.support-27.0.2.aar#class-abi/com.support-27.0.2.aar-abi.jar

How can I filter and keep only the #class-abi deps? Also is there a scenario where the output of the two flavours will differ?

added a filter to only include rules which have non-null sourcePathToOutput, same behaviour is in ClasspathMacro.

facebook-github-bot · 2018-02-22T01:43:49Z

@raviagarwal7 has updated the pull request. View: changes

facebook-github-bot

@styurin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

For android lint genrule, dependencies class-abi is enough. classpath_abi was added in facebook/buck#1780

styurin · 2018-02-22T21:10:17Z

src/com/facebook/buck/rules/macros/ClasspathAbiMacroExpander.java

+ * @param ruleResolver BuildRuleResolver
+ * @return class abi jar or output jar if not found
+ */
+ private SourcePath getJarPath(BuildRule rule, BuildRuleResolver ruleResolver) {


This needs to be annotated with @Nullable.

styurin · 2018-02-22T21:11:22Z

test/com/facebook/buck/rules/macros/ClasspathAbiMacroExpanderTest.java

+ String.format(
+ "%s" + File.separator + "%s",
+ filesystem.getRootPath(),
+ "buck-out/gen/cheese/cake#class-abi/cake-abi.jar"));


These tests are failing on Windows. Our CI doesn't run these tests for some reason (we're investigating), but it looks like it's failing because of the path separators.

using new File.("file-path-string")toPath() now, hopefully this would work fine on windows.

Running android lint on a target only needs dependent libraries class-abi, i.e doesn’t need to rerun lint if there are no api changes in the dependent libraries. Using classpath_abi returns class-abi jars of the dependencies instead of the class jar. (Buck pr - facebook/buck#1780) Usage: $(@classpath_abi :<target_path>)

facebook-github-bot · 2018-02-22T22:45:22Z

@raviagarwal7 has updated the pull request. View: changes, changes since last import

facebook-github-bot

@styurin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

styurin · 2018-02-23T17:55:57Z

test/com/facebook/buck/rules/macros/ClasspathAbiMacroExpanderTest.java

+ private ProjectFilesystem filesystem;
+
+ @Before
+ public void createMacroExpander() {


this is usually called setUp. It also does more than creating macro expander.

styurin · 2018-02-23T17:57:16Z

test/com/facebook/buck/rules/macros/ClasspathAbiMacroExpanderTest.java

+ }
+
+ @Test
+ public void shouldIncludeARuleIfNothingIsGiven() throws Exception {


test methods usually start with test so that the method name is a verb, right now it's a statement

facebook-github-bot · 2018-02-23T18:34:34Z

@raviagarwal7 has updated the pull request. View: changes, changes since last import

kageiit · 2018-02-28T02:56:23Z

cc @styurin the comments are addressed. Any update on this?

facebook-github-bot

@styurin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Running android lint on a target only needs dependent libraries class-abi, i.e doesn’t need to rerun lint if there are no api changes in the dependent libraries. Using classpath_abi returns class-abi jars of the dependencies instead of the class jar. (Buck pr - facebook/buck#1780) Usage: $(@classpath_abi :<target_path>)

Running android lint on a target only needs dependent libraries class-abi, i.e doesn’t need to rerun lint if there are no api changes in the dependent libraries. Using classpath_abi returns class-abi jars of the dependencies instead of the class jar. (Buck pr - facebook/buck#1780) Usage: $(@classpath_abi :<target_path>) To enable using classpath_abi, add the experimental extension ``` okbuck { experimental { lintWithClasspathAbi = true } } ```

* Use classpath_abi macro instead of classpath in the lint genrule. Running android lint on a target only needs dependent libraries class-abi, i.e doesn’t need to rerun lint if there are no api changes in the dependent libraries. Using classpath_abi returns class-abi jars of the dependencies instead of the class jar. (Buck pr - facebook/buck#1780) Usage: $(@classpath_abi :<target_path>) To enable using classpath_abi, add the experimental extension ``` okbuck { experimental { lintWithClasspathAbi = true } } ```

facebook-github-bot added the CLA Signed label Feb 21, 2018

raviagarwal7 mentioned this pull request Feb 21, 2018

Adding support for getting the abi classpath in genrule macros #1635

Closed

styurin suggested changes Feb 21, 2018

View reviewed changes

raviagarwal7 added 2 commits February 21, 2018 15:14

Fix lint and add some doc

9ae8a09

raviagarwal7 force-pushed the ravi/abi-jar branch from 60eea03 to 9ae8a09 Compare February 21, 2018 23:22

raviagarwal7 changed the title ~~Add support for @abi_jar_path macro~~ Add support for @classpath_abi macro Feb 21, 2018

styurin suggested changes Feb 21, 2018

View reviewed changes

raviagarwal7 commented Feb 21, 2018

View reviewed changes

raviagarwal7 added 2 commits February 21, 2018 15:45

Revert styling changes and add some javadoc.

6bc8241

Don’t add duplicate jar’s to classpath_abi

d21495a

facebook-github-bot reviewed Feb 22, 2018

View reviewed changes

raviagarwal7 added a commit to uber/okbuck that referenced this pull request Feb 22, 2018

Use classpath_abi macro instead of classpath in lint genrule.

f0948ce

For android lint genrule, dependencies class-abi is enough. classpath_abi was added in facebook/buck#1780

raviagarwal7 mentioned this pull request Feb 22, 2018

Use classpath_abi macro instead of classpath in lint genrule. uber/okbuck#590

Merged

styurin suggested changes Feb 22, 2018

View reviewed changes

Try to fix tests on windows

044df00

facebook-github-bot reviewed Feb 23, 2018

View reviewed changes

styurin suggested changes Feb 23, 2018

View reviewed changes

code review comments

16cf531

facebook-github-bot reviewed Feb 28, 2018

View reviewed changes

facebook-github-bot added the Import Started label Feb 28, 2018

facebook-github-bot closed this in 89681a5 Feb 28, 2018

kageiit deleted the ravi/abi-jar branch February 28, 2018 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for @classpath_abi macro #1780

Add support for @classpath_abi macro #1780

raviagarwal7 commented Feb 21, 2018 •

edited

Loading

styurin left a comment

facebook-github-bot commented Feb 21, 2018

raviagarwal7 commented Feb 21, 2018

styurin Feb 21, 2018

raviagarwal7 Feb 21, 2018

styurin Feb 21, 2018

raviagarwal7 Feb 21, 2018

raviagarwal7 Feb 21, 2018 •

edited

Loading

kageiit Feb 22, 2018

raviagarwal7 Feb 22, 2018

facebook-github-bot commented Feb 22, 2018

facebook-github-bot left a comment

styurin Feb 22, 2018

raviagarwal7 Feb 22, 2018

styurin Feb 22, 2018

raviagarwal7 Feb 22, 2018

facebook-github-bot commented Feb 22, 2018

facebook-github-bot left a comment

styurin Feb 23, 2018

raviagarwal7 Feb 23, 2018

styurin Feb 23, 2018

raviagarwal7 Feb 23, 2018

facebook-github-bot commented Feb 23, 2018

kageiit commented Feb 28, 2018

facebook-github-bot left a comment

Add support for @classpath_abi macro #1780

Add support for @classpath_abi macro #1780

Conversation

raviagarwal7 commented Feb 21, 2018 • edited Loading

styurin left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 21, 2018

raviagarwal7 commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raviagarwal7 Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 22, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 22, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 23, 2018

kageiit commented Feb 28, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

raviagarwal7 commented Feb 21, 2018 •

edited

Loading

raviagarwal7 Feb 21, 2018 •

edited

Loading