-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Update Default Target Directory for MetadataCopy Task #580
Update Default Target Directory for MetadataCopy Task #580
Conversation
@@ -99,7 +99,7 @@ graalvmNative { | |||
// Copies metadata collected from tasks into the specified directories. | |||
metadataCopy { | |||
inputTaskNames.add("test") // Tasks previously executed with the agent attached. | |||
outputDirectories.add("src/main/resources/META-INF/native-image") | |||
outputDirectories.add("src/main/resources/META-INF/native-image/<groupId>/<artifactId>/") // Replace <groupId> and <artifactId> with GAV coordinates of your project |
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.
Is the trailing forward slash correct? We used to not have it.
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.
Since src/main/resources/META-INF/native-image/<groupId>/<artifactId>/
is directory, I don't see any difference if we use the trailing forward slash or not. It should work in any case, and I think it doesn't change anything. We can remove it if it is necessary
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.
The original issue as well as this PR is confusing. The issue says that metadata-copy
copies to META-INF/native-image
, but there's no such default in Gradle. I see that there's one in Maven, but I'm wondering if it's a good idea.
There was a reason for not adding a default in the Gradle version (which I think predates the Maven one), which is to avoid having overlapping outputs (basically preventing the build from being up-to-date), but also because we don't really know what was used by the agent to collect metadata, so adding this explicitly could overwrite legit metadata.
So I think we should either:
- remove the default from the Maven plugin, then document how to set the target directory for metadata copy
- or change the Gradle plugin so that it also introduces a default value (which again I'm not sure is a good idea)
Currently the PR mixes a bit the 2, it doesn't really fix the consistency issue between both plugins or the documentation.
But it will override metadata only if we explicitly call
Yes, I agree that we should sync Maven and Gradle solutions. |
When I opened the ticket, I was not aware that this is another part where our Maven and Gradle plugins diverge. Yes, we should fix that and align those. This PR is still an improvement: while it doesn't fix the misalignment, it improves the recommendation. Does it make sense to merge this and align the plugins in a follow up (after a discussion it seems)? |
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.
LGTM, thanks!
As described in this issue we should specify default output directory for metadataCopy task according to the recommendation from GraalVM website
Note: currently we don't have default value for the Gradle plugin, while we do have one for the Maven