Fixed
Status Update
Comments
uc...@google.com <uc...@google.com> #2
Thank you for your feedback. Team may reach out for more feedback in reproducing or triaging this issue.
je...@google.com <je...@google.com>
ne...@soundcloud.com <ne...@soundcloud.com> #3
In case anyone else is having this problem, we are using the following workaround
```
afterEvaluate {
tasks.named("compileDebugAidl").configure {
// Once the generated source has been generated, `doLast` will go through and remove the fully qualified path.
doLast {
outputs.files.forEach { directory ->
directory.traverse(type: groovy.io.FileType.FILES) { file ->
removeFullyQualifiedPath(file)
}
}
}
}
}
/**
* For a given file, this rewrites the file to exclude the line "Original file:" which includes the fully
* qualified path of the original file used to generate this file.
* This is to allow CI and local builds (which have different fully qualified paths) to share the caching of
* this compiled module.
*/
@groovy.transform.CompileStatic
static def removeFullyQualifiedPath(File file) {
file.setText((file as String[]).findAll { !it.startsWith('Original file:') }.join('\n'), 'utf-8')
}
```
We also moved the generation of this AIDL file to a new empty module and noticed that the up to date check went from 2 seconds to .025 seconds. I'm assuming this is because the up to date check had to scan all the java files in a large java module in order to determine if it was up to date because we were setting the aidl.srcDirs to the javaSrcDirs.
```
android.sourceSets {
main {
aidl.srcDirs javaSrcDirs
}
}
```
```
afterEvaluate {
tasks.named("compileDebugAidl").configure {
// Once the generated source has been generated, `doLast` will go through and remove the fully qualified path.
doLast {
outputs.files.forEach { directory ->
directory.traverse(type: groovy.io.FileType.FILES) { file ->
removeFullyQualifiedPath(file)
}
}
}
}
}
/**
* For a given file, this rewrites the file to exclude the line "Original file:" which includes the fully
* qualified path of the original file used to generate this file.
* This is to allow CI and local builds (which have different fully qualified paths) to share the caching of
* this compiled module.
*/
@groovy.transform.CompileStatic
static def removeFullyQualifiedPath(File file) {
file.setText((file as String[]).findAll { !it.startsWith('Original file:') }.join('\n'), 'utf-8')
}
```
We also moved the generation of this AIDL file to a new empty module and noticed that the up to date check went from 2 seconds to .025 seconds. I'm assuming this is because the up to date check had to scan all the java files in a large java module in order to determine if it was up to date because we were setting the aidl.srcDirs to the javaSrcDirs.
```
android.sourceSets {
main {
aidl.srcDirs javaSrcDirs
}
}
```
ne...@soundcloud.com <ne...@soundcloud.com> #4
Oops, small bug, use this line instead:
file.setText((file as String[]).findAll { !it.contains('Original file:') }.join('\n'), 'utf-8')
file.setText((file as String[]).findAll { !it.contains('Original file:') }.join('\n'), 'utf-8')
je...@google.com <je...@google.com> #5
Hi,
Can you tell us the name of that file and/or where it is located?
Can you tell us the name of that file and/or where it is located?
[Deleted User] <[Deleted User]> #6
Screenshot attached (I worked with SoundCloud on devising a workaround for this problem).
hu...@google.com <hu...@google.com> #7
This absolute path is produced by the AIDL tool, so I think the AGP will need to remove that path from the output of the tool.
je...@google.com <je...@google.com>
ji...@google.com <ji...@google.com> #8
Hi,
I and smoreland@ have been working on the aidl compiler recently. BTW, the printout of the absolute path has been there over 2 years. Is this a regression or just newly discovered?
I and smoreland@ have been working on the aidl compiler recently. BTW, the printout of the absolute path has been there over 2 years. Is this a regression or just newly discovered?
[Deleted User] <[Deleted User]> #9
I presume simply newly discovered. Gradle Enterprise recently released a feature that makes it very easy to see which exact file inputs to a task led to a remote build cache miss, and why.[1] I work at Gradle and helped SoundCloud discover this issue in the relevant task, and also devise the workaround. Happy to provide any additional context, if that might be helpful.
[1]https://docs.gradle.com/enterprise/tutorials/task-inputs-comparison/
[1]
sm...@google.com <sm...@google.com> #10
I believe the reason we haven't hit this before on Android platform side is that we always use relative paths in our build system. Before we merge Jiyong's fix for this ([1]), is there a way that you can use the exact same commands on the server side build environment that a client would use (some sort of relative path)? I would expect this to remove a whole class of possible cache-misses.
[1]https://android-review.googlesource.com/c/platform/system/tools/aidl/+/898853/
[1]
hu...@google.com <hu...@google.com> #11
Yes, I think it is possible (and actually better) to fix this issue from the Android Gradle plugin side: If we pass the relative path of an .aidl source file to the AIDL compiler, then it will output that relative path in the generated .java file.
@Jean-Luc: In AidlProcessor.java, you can try changing builder.addArgs(path.toAbsolutePath().toString()); to a relative path (we'll need to pass the project's directory path from an upstream method as well in order to compute the relative path), and then run AidlTest to check the results.
@Jean-Luc: In AidlProcessor.java, you can try changing builder.addArgs(path.toAbsolutePath().toString()); to a relative path (we'll need to pass the project's directory path from an upstream method as well in order to compute the relative path), and then run AidlTest to check the results.
je...@google.com <je...@google.com> #12
I took a look and it looks like the present working directory can be outside the source directory.
So if you give it a relative path of com/android/vending/billing/IInAppBillingService.aidl from the project source, it will run 'aidl com/android/vending/billing/IInAppBillingService.aidl' from some (not necessarily fixed) other directory.
That is why we use absolute paths.
This means it is not possible to use relative paths unless some sort of "set working directory" option is implemented in aidl.
So if you give it a relative path of com/android/vending/billing/IInAppBillingService.aidl from the project source, it will run 'aidl com/android/vending/billing/IInAppBillingService.aidl' from some (not necessarily fixed) other directory.
That is why we use absolute paths.
This means it is not possible to use relative paths unless some sort of "set working directory" option is implemented in aidl.
je...@google.com <je...@google.com> #13
Patch with relative path:
ag/6377482
ag/6377482
ap...@google.com <ap...@google.com> #14
Project: platform/system/tools/aidl
Branch: master
commit 54fa1e0b02f136fb7d545d4a0e800d8ea66f22fb
Author: Jiyong Park <jiyong@google.com>
Date: Fri Feb 08 10:03:20 2019
Don't printout absolute path to the original source
They are make Java files generated from the same *.aidl file but in
different paths be different, which might cause trouble to the build
cache server.
The path to the original file doesn't seem to be necessary because the
path is already encoded in the intermediate directory where the
generated files are (e.g.,
out/soong/.intermediate/<path>/<module>/gen/aidl/<path>/IXxxx.java)
Bug: 121251997
Test: aidl_unittests
Change-Id: I337a217fef7852fe0e5f683cc239508e982f4fc4
M aidl.cpp
M aidl_unittest.cpp
M ast_java.cpp
M ast_java.h
M generate_java.cpp
M generate_java.h
M tests/test_data_example_interface.cpp
M tests/test_data_string_constants.cpp
https://android-review.googlesource.com/898853
https://goto.google.com/android-sha1/54fa1e0b02f136fb7d545d4a0e800d8ea66f22fb
Branch: master
commit 54fa1e0b02f136fb7d545d4a0e800d8ea66f22fb
Author: Jiyong Park <jiyong@google.com>
Date: Fri Feb 08 10:03:20 2019
Don't printout absolute path to the original source
They are make Java files generated from the same *.aidl file but in
different paths be different, which might cause trouble to the build
cache server.
The path to the original file doesn't seem to be necessary because the
path is already encoded in the intermediate directory where the
generated files are (e.g.,
out/soong/.intermediate/<path>/<module>/gen/aidl/<path>/IXxxx.java)
Bug: 121251997
Test: aidl_unittests
Change-Id: I337a217fef7852fe0e5f683cc239508e982f4fc4
M aidl.cpp
M aidl_unittest.cpp
M ast_java.cpp
M ast_java.h
M generate_java.cpp
M generate_java.h
M tests/test_data_example_interface.cpp
M tests/test_data_string_constants.cpp
ji...@google.com <ji...@google.com>
ne...@soundcloud.com <ne...@soundcloud.com> #15
Thanks for the quick fix! Do we know in which version of the Android Gradle Plugin this will be shipped in?
hu...@google.com <hu...@google.com> #16
This was fixed in our build tools, so it will not be available until the AGP upgrades its default build tools version, which may take a while because I don't know when the next build tools version will be released (it's currently at 28.0.3).
@Jean-Luc: Would it be possible to implement a workaround in the AGP (and remove it later once we have updated our build tools) so that the users get the fix sooner. The workaround will be to remove the comment with an absolute path from the AIDL output.
@Jean-Luc: Would it be possible to implement a workaround in the AGP (and remove it later once we have updated our build tools) so that the users get the fix sooner. The workaround will be to remove the comment with an absolute path from the AIDL output.
hu...@google.com <hu...@google.com> #17
@1: As regards the slow UP-TO-DATE check of AidlCompile, I've filed https://github.com/gradle/gradle/issues/8559 . Thanks for letting us know!
je...@google.com <je...@google.com> #18
I'll do that ASAP
ne...@soundcloud.com <ne...@soundcloud.com> #19
Thanks for fixing this. We're testing out AGP 3.5 RC 1 and noticed that the generated java file did not contain the absolute path to the original source file.
We are, however, experiencing a different cache miss. It seems the aidlExecutableProvider property for the AidlCompile task is different on our local machines vs on our CI system. Could this be due to the fact that most of our developers run mac OS systems while our CI is using unix systems? Is the aidlExecutableProvider platform dependent?
Should I open a new bug for this issue or keep it in this thread?
We are, however, experiencing a different cache miss. It seems the aidlExecutableProvider property for the AidlCompile task is different on our local machines vs on our CI system. Could this be due to the fact that most of our developers run mac OS systems while our CI is using unix systems? Is the aidlExecutableProvider platform dependent?
Should I open a new bug for this issue or keep it in this thread?
hu...@google.com <hu...@google.com> #20
> Is the aidlExecutableProvider platform dependent?
That's a good question and will decide the expected behavior in this case.
@Jiyong: Is it guaranteed that an AIDL executable on one platform always produces the same output as another AIDL executable on a different platform, given the same inputs and build tools revision?
That's a good question and will decide the expected behavior in this case.
@Jiyong: Is it guaranteed that an AIDL executable on one platform always produces the same output as another AIDL executable on a different platform, given the same inputs and build tools revision?
hu...@google.com <hu...@google.com> #21
bu...@google.com <bu...@google.com> #22
To be honest, I don't know if the executable is different but that would make sense.
I would argue that the current way is the correct, in the sense that there is no other way to guarantee that two different (aidl) binaries would produce the same result.
But, if we deemed that is incorrect we can change the @inputs of the task to just acknowledge that the aidl components are present (so the inputs would change to a boolean that represents if the file returned by the provider exists or not) + verify the build version tools (which is the best proxy I can think of to represent that we are using the same aidl behavior, across platforms).
I would argue that the current way is the correct, in the sense that there is no other way to guarantee that two different (aidl) binaries would produce the same result.
But, if we deemed that is incorrect we can change the @inputs of the task to just acknowledge that the aidl components are present (so the inputs would change to a boolean that represents if the file returned by the provider exists or not) + verify the build version tools (which is the best proxy I can think of to represent that we are using the same aidl behavior, across platforms).
[Deleted User] <[Deleted User]> #23
I would argue that it is incorrect to have different behavior depending on which operating system you run a build on. The build should be reproducible in different environments. If your aidl binary might produce different results depending on OS, that sounds like a bug to me.
I reported similar issue with aapt2, and it was determined that you shouldn't use a path to the binary as the input, but the version of that binary. Seehttps://issuetracker.google.com/issues/134838482
I reported similar issue with aapt2, and it was determined that you shouldn't use a path to the binary as the input, but the version of that binary. See
ji...@google.com <ji...@google.com> #24
> Is the aidlExecutableProvider platform dependent?
No, I don't think so. But I might be missing something. Could you let us know how the outputs are different in your case? (sharing the output files perhaps)
No, I don't think so. But I might be missing something. Could you let us know how the outputs are different in your case? (sharing the output files perhaps)
ne...@soundcloud.com <ne...@soundcloud.com> #25
> Could you let us know how the outputs are different in your case?
Now that the fully qualified path is not printed in the generated java file, the output files the same as far as we can tell.
Now that the fully qualified path is not printed in the generated java file, the output files the same as far as we can tell.
ji...@google.com <ji...@google.com> #26
Thanks, but then comment#25 seems to be contradicting with comment#19 . I am not sure what is going on in the gradle side.
ne...@soundcloud.com <ne...@soundcloud.com> #27
Ah, sorry for the confusion.
The aidlExecutableProvider property for the AidlCompile task is an input to the task. The inputs of the task do not match therefore the computed cache key differs and cannot be re-used.
As far as we can tell, the outputs of the task are the same. Tasks such as the javacompile task which depends on the outputs of this task have the same inputs.
See the image in comment#19 for details on the cache miss.
The aidlExecutableProvider property for the AidlCompile task is an input to the task. The inputs of the task do not match therefore the computed cache key differs and cannot be re-used.
As far as we can tell, the outputs of the task are the same. Tasks such as the javacompile task which depends on the outputs of this task have the same inputs.
See the image in
hu...@google.com <hu...@google.com> #28
Thanks for the discussion. This confirms that the aidl processor's contents should not be considered as input. I have filed Bug 138920846 for this.
Description
I have attached a screenshot of a build scan with task inputs comparison comparison. It shows that the relative path of the generated file as well as the contents are being considered for cacheability, but the contents differ due to the header containing the absolute path of the original file.
Using Gradle 5.0
Android Gradle Plugin 3.3.0-rc02
Java Source compatibility 1.8
Java Target compatibility 1.8
Unrelated, but the up-to-date check of the compileDebugAidl task can take up to 2 seconds which seems rather slow for an up to date check.