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

skip non-class files in the exclusions processing for MergeJars #1239

Merged

Conversation

vinnybod
Copy link
Contributor

@vinnybod vinnybod commented Sep 7, 2024

Note: This is a dupe of #1125 . I had to reopen the PR from a different GH org.


This is sort of an addendum to #1106 and #1107
In the previous PR, I added an exception for COPYRIGHT and NOTICE since it was the same kind of use case as the existing LICENSE exception.

The issue that I am having is that I have files that should be present in all of the jars being published in a repo that are not COPYRIGHT, NOTICE, or LICENSE. For example a project-version.properties file that contains info about the build.

Changing the readExcludedFileNames function to only process *.class files allows for these files to propagate to the final jar. It also still passes all existing test cases.

The only test for this functionality is specifically looking at .class files.https://github.com/bazelbuild/rules_jvm_external/blob/master/tests/com/github/bazelbuild/rules_jvm_external/jar/MergeJarsTest.java#L221

I am open to other solutions, but this seems to have fixed a lot of issues for my use case without breaking any existing functionality that is being tested for.

Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

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

Let's see what happens. LGTM.

@shs96c shs96c merged commit 0a8dd11 into bazel-contrib:master Sep 9, 2024
8 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.

2 participants