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

Manually relocate snakeyaml multi-release classes. #1008

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

schmist
Copy link
Contributor

@schmist schmist commented Nov 2, 2023

The multi-release classes for the relocated snakeyaml dependency need to be relocated manually due to a bug in the Maven Shade plugin. The plugin only relocates the content but not the package structure itself. See https://issues.apache.org/jira/browse/MSHADE-406. This fixes #1007.

The multi-release classes for the relocated snakeyaml dependency need to be relocated manually due to a bug in the Maven Shade plugin. The plugin only relocates the content but not the package structure itself. See https://issues.apache.org/jira/browse/MSHADE-406.
Copy link

what-the-diff bot commented Nov 2, 2023

PR Summary

  • Modification of Project Configuration File
    The project's set-up file (pom.xml) has been adjusted. This change ensures that certain code components (shaded snakeyaml multi-release classes in this case) are moved to the appropriate package (folder structure in simple terms). This was necessary due to an issue in the tool our project uses for creating a standalone version of the project (Maven Shade plugin), which was not correctly handling the movement and organization of these pieces of code. This update helps to resolve that issue by manually directing the code to the correct places.

@schmist schmist changed the title Manually relocate snakeyaml multi-release classes, fixes #1007. Manually relocate snakeyaml multi-release classes. Nov 2, 2023
pom.xml Outdated
Comment on lines 298 to 299
<pattern>META-INF/versions/9/org.yaml.snakeyaml</pattern>
<shadedPattern>META-INF/versions/9/net.datafaker.shaded.snakeyaml</shadedPattern>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious whether there is a more generic solution which could handle other versions once they appear...
Otherwise any new version will lead to the same issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be an (undocumented way) to do this, I am currently looking at the Maven plugin code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The follwing commit should address your concern. Now all versions of the org.yaml.snakeyaml package are being relocated. Although new versions will not come around very often and I hope that the actual bug in the Maven plugin will get fixed sooner or later so that this workaround can be removed.

@schmist schmist requested a review from snuyanzin November 2, 2023 21:15
Copy link
Collaborator

@snuyanzin snuyanzin left a comment

Choose a reason for hiding this comment

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

I noticed that in shaded classes (e.g. Logger) there is a package name
package META-INF.versions.9.org.yaml.snakeyaml.internal;
i.e. it contains - in META-INF which is not supported symbol in package names...
I wonder how it is supposed to work?

@schmist
Copy link
Contributor Author

schmist commented Nov 6, 2023

package META-INF.versions.9.org.yaml.snakeyaml.internal

Where do you see this? Inside the packaged jar? It should be translated to the correct package name when packaging. The META-INF is only necessary to locate the correct folder.

@snuyanzin
Copy link
Collaborator

yes, inside the packaged jar
yep, probably you are right
however right now i'm not able to test end2end case.
Can you confirm that this change makes it working?

@schmist
Copy link
Contributor Author

schmist commented Nov 6, 2023

It probably works because the relocation I added is only used to move the classes into the correct package. The package name inside the class files is not replaced since the provided relocation path (including META-INF) does not match. Since the buggy Maven Shade plugin has done the renaming already (but not the relocation into the correct package, see linked bug report), we should be on the save side. I agree that it is not a clean solution but the result is a working and correct multi-release jar. This workaround can be removed as soon as the bug in the Plugin is fixed.

I can also confirm that with this fix, the issue is gone for our use case and datafaker is still doing its job.

@schmist
Copy link
Contributor Author

schmist commented Nov 6, 2023

It seems that they are currently working on a fix: apache/maven-shade-plugin#202

@snuyanzin
Copy link
Collaborator

thanks for checking
yes, probably it would make sense to remove this from pom file once there will be a new shade plugin version with the fix available

@snuyanzin snuyanzin merged commit a7ce047 into datafaker-net:main Nov 6, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relocated snakeyaml package not correct in META-INF/versions
2 participants