Skip to content
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

Update Default Target Directory for MetadataCopy Task #580

Merged

Conversation

dnestoro
Copy link
Collaborator

@dnestoro dnestoro commented Mar 6, 2024

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

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 6, 2024
@fniephaus fniephaus requested a review from olpaw March 7, 2024 07:35
@@ -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
Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

@melix melix left a 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.

@dnestoro
Copy link
Collaborator Author

dnestoro commented Mar 7, 2024

so adding this explicitly could overwrite legit metadata

But it will override metadata only if we explicitly call metadataCopy task? Otherwise the metadata will be stored as usual into directory specified in agent-output-dir. The point here is that we should make metadataCopy task coping metadata to the directory we are suggesting in our docs (so that user can just say ./gradlew metadataCopy and all metadata will be on recommended location). If I am not missing something.

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)

Yes, I agree that we should sync Maven and Gradle solutions.

@fniephaus
Copy link
Member

The original issue as well as this PR is confusing

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)?

Copy link
Member

@fniephaus fniephaus left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@dnestoro dnestoro merged commit 6db59ba into graalvm:master Mar 21, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants