Skip to content

bnd-maven plugin: delete existing MANIFEST.MF from previous build #6222

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 22, 2024
Merged

bnd-maven plugin: delete existing MANIFEST.MF from previous build #6222

merged 1 commit into from
Aug 22, 2024

Conversation

@chrisrueger
Copy link
Contributor

@chrisrueger chrisrueger commented Aug 19, 2024 •
edited
Loading

Based on comment #6221 (comment)

This fixes a problem where a 'mvn process-classes' picked up target/classes/META-INF/MANIFEST.MF from a previous build and produced a new MANIFEST.MF based on the results of the previous build... which should not happen.

We now delete the existing target/classes/META-INF/MANIFEST.MF if it exists.

Now this gives consistent results for the following case:

mvn clean process-classes
mvn clean process-classes

or

mvn clean process-classes
followed by a
mvn process-classes

@pkriens I wonder how this PR affects the statement in

bnd/maven-plugins/bnd-maven-plugin/README.md

Line 111 in c1bb165

### IMPORTANT NOTE about Maven JAR|WAR Plugin

@chrisrueger chrisrueger requested a review from pkriens August 19, 2024 19:46
@chrisrueger chrisrueger mentioned this pull request Aug 19, 2024
Closed
@chrisrueger chrisrueger changed the title delete existing MANIFEST.MF from previous build bnd-maven plugin: delete existing MANIFEST.MF from previous build Aug 19, 2024
@Marcono1234
Copy link

Marcono1234 commented Aug 19, 2024 •
edited
Loading

Is it actually guaranteed that target/classes/META-INF/MANIFEST.MF comes from a previous build by bnd-maven-plugin? I have not tested it yet, but what if a user has a src/main/resources/META-INF/MANIFEST.MF and relies on bnd-maven-plugin enhancing / extending it? (Not sure though if that is an intended / supported use case.)

@chrisrueger
Copy link
Contributor Author

chrisrueger commented Aug 20, 2024 •
edited
Loading

but what if a user has a src/main/resources/META-INF/MANIFEST.MF and relies on bnd-maven-plugin enhancing / extending it? (Not sure though if that is an intended / supported use case.)

This is a very good question I was concerned about.
If that is the case, then the question is how to distinguish between those cases.

Do you think the scenarios here mention the use case you outlined?

@chrisrueger chrisrueger marked this pull request as draft August 20, 2024 13:22
bjhargrave
bjhargrave requested changes Aug 20, 2024
View reviewed changes
@bjhargrave
Copy link
Member

bjhargrave commented Aug 20, 2024

You also need to remove the merge commit. Only normal commits in a PR.

@chrisrueger chrisrueger force-pushed the fix-bnd-maven-plugin-manifest-in-target branch from 8030bbc to 9d125eb Compare August 20, 2024 19:26
@chrisrueger chrisrueger marked this pull request as ready for review August 20, 2024 19:28
bjhargrave
bjhargrave requested changes Aug 20, 2024
View reviewed changes
@bjhargrave
Copy link
Member

bjhargrave commented Aug 21, 2024

This all looks good to me. The final thing needed is to squash all this into a single commit. Thanks for the work on this PR!

@chrisrueger @bjhargrave
add option 'deleteExistingManifest'
6940c01 
Signed-off-by: Christoph <[email protected]>

delete existing MANIFEST.MF from previous build

This fixes a problem where a 'mvn process-classes' picked up target/classes/META-INF/MANIFEST.MF from a previous build and produced a new MANIFEST.MF based on the results of the previous build... which should not happen.

Now this gives consistent results for the following case:

mvn clean process-classes
followed by a
mvn process-classes

Signed-off-by: Christoph Rueger <[email protected]>

add option 'deleteExistingManifest'

Signed-off-by: Christoph Rueger <[email protected]>

Update maven-plugins/bnd-maven-plugin/README.md

Signed-off-by: Christoph <[email protected]>

use getManifestPath()

Signed-off-by: Christoph Rueger <[email protected]>

move deleteExistingManifest outside if

Since now it is independent of classesDir and depends on manifestPath instead

Signed-off-by: Christoph Rueger <[email protected]>

Update maven-plugins/bnd-maven-plugin/src/main/java/aQute/bnd/maven/plugin/AbstractBndMavenPlugin.java

Signed-off-by: Christoph <[email protected]>

Update maven-plugins/bnd-maven-plugin/README.md

Signed-off-by: Christoph <[email protected]>

Update maven-plugins/bnd-maven-plugin/README.md

Signed-off-by: Christoph <[email protected]>

Update maven-plugins/bnd-maven-plugin/README.md

Signed-off-by: Christoph <[email protected]>

Update maven-plugins/bnd-maven-plugin/src/main/java/aQute/bnd/maven/plugin/AbstractBndMavenPlugin.java

Signed-off-by: Christoph <[email protected]>
Co-Authored-By: BJ Hargrave <[email protected]>
@chrisrueger chrisrueger force-pushed the fix-bnd-maven-plugin-manifest-in-target branch from 6235764 to 6940c01 Compare August 21, 2024 12:41
@Marcono1234
Copy link

Marcono1234 commented Aug 21, 2024

Do you think the scenarios here mention the use case you outlined?

No, I think they all cover how to configure maven-jar-plugin / maven-war-plugin to use the MANIFEST.MF generated by bnd. They don't seems to cover using bnd to process an existing MANIFEST.MF, so at least to me it really looks likes this is not an officially supported use case.

@pkriens
Copy link
Member

pkriens commented Aug 22, 2024

LGTM

@bjhargrave bjhargrave merged commit 686b281 into bndtools:master Aug 22, 2024
@chrisrueger chrisrueger mentioned this pull request Sep 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjhargrave bjhargrave bjhargrave approved these changes

@pkriens pkriens Awaiting requested review from pkriens

+1 more reviewer

@laeubi laeubi laeubi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chrisrueger @Marcono1234 @bjhargrave @pkriens @laeubi