Skip to content

Add BndManifest support for every Jar task#79

Merged
nedtwigg merged 8 commits intodiffplug:masterfrom
hacki11:ft-openbndmanifest
Oct 3, 2018
Merged

Add BndManifest support for every Jar task#79
nedtwigg merged 8 commits intodiffplug:masterfrom
hacki11:ft-openbndmanifest

Conversation

@hacki11
Copy link
Contributor

@hacki11 hacki11 commented Sep 9, 2018

This is a feature to support BndManifest configuration for different Jar tasks.

  1. Additional task must be applied ('jar' is applied by default)
osgiBndManifest {
    includeTask 'shadowJar'
}
  1. Multiple tasks must be applied
osgiBndManifest {
    includeTasks 'shadowJar', 'anotherJar'
}
  1. Only a custom task must be applied (no jar wanted)
osgiBndManifest {
    includeTasks = ['shadowJar']
}
  1. It is possible to copy the Manifest.MF from another task than jar:
osgiBndManifest {
    copyFromTask 'shadowJar'
    copyTo 'META-INF/MANIFEST.MF'
    includeTask 'shadowJar'
}

@hacki11 hacki11 requested a review from nedtwigg September 9, 2018 16:37
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

This looks great overall, but needs a little more work before merge.

The main problem is the interaction between multiple Jar tasks and copying their manifests to one extension.copyTo place. They're going to overwrite each other, and it's not clear what the "right" behavior is.

A second problem is that some users might be using Goomph on their main jar but not their auxiliary jars, and they will be surprised when that changes if they didn't read the changelog carefully (which they don't for a minor version bump).

One solution is if BndManifestExtension adds a boolean field applyToAllJarTasks which defaults to false. Then we can throw an error if (applyToAllJarTasks && copyTo != null). Would that workaround address your needs?

@hacki11
Copy link
Contributor Author

hacki11 commented Sep 10, 2018

Thanks for your review, you brought up good points!

You are right, we should not change default behavior.
I will come up with an alternative, more configurable aproach we can discuss in a few days.

@nedtwigg
Copy link
Member

Just FYI, Eclipse 4.9 is releasing September 19, 2018, so I plan to cut a new release that day. Easy to cut a release before or after too, so no rush.

- added configuration properties to configure additional tasks
- added a copyFromTask property
@hacki11
Copy link
Contributor Author

hacki11 commented Sep 23, 2018

i had some time and thought about a possible solution which is more generic. i have committed my proposal.

  • Default behavior is preserved again
  • jar task is used by default
  • mergeWithExisiting is a global property for all applied tasks
  • copyFromTask property added (jar is default)

Use Cases:

  1. Additional task must be applied ('jar' is applied by default)
osgiBndManifest {
    includeTask 'shadowJar'
}
osgiBndManifest {
    includeTasks 'shadowJar', 'anotherJar'
}
  1. Only a custom task must be applied (no jar wanted)
osgiBndManifest {
    includeTasks = ['shadowJar']
}
  1. Solving copyTo with an additional property copyFromTask to solve the problem with multiple copies from different tasks. The manifest of the configured task is used (jar by default)
osgiBndManifest {
    copyFromTask 'shadowJar'
    copyTo 'META-INF/MANIFEST.MF'
    includeTask 'shadowJar'
}

I have added some checks to avoid misconfiguration like

  • define copyTo but setting copyFromTask to null
  • using tasks which are not extending Jar
  • using a copyFromTask which is not includedTasks

If you have some time i would be happy to get some feedback and review for this proposal.
I will add more unit tests after we are done with designing the configuration.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

This looks great! I have only a few very minor changes to request, I'll merge and release as soon as you are ready.

SourceSetOutput main = javaConvention.getSourceSets().getByName(SourceSet.MAIN_SOURCE_SET_NAME).getOutput();
// delete empty folders so that bnd doesn't make Export-Package entries for them
StringBuilder includeresource = new StringBuilder();
Set<String> includeresource = new HashSet<>();
Copy link
Member

Choose a reason for hiding this comment

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

HashSet<> has undefined order. Previous solution had determined order. I think Bnd is sorting these, so it shouldn't matter, but safer if this is either ArrayList<> or LinkedHashSet. You call which.

@hacki11
Copy link
Contributor Author

hacki11 commented Sep 24, 2018

good eye, i should definitely add some todos while changing code for debugging.
I will do a check on the includeresource topic, had some issues with here with duplicate files.

@nedtwigg
Copy link
Member

Just a few changes left :) I'll merge and release as soon as you're ready.

@hacki11
Copy link
Contributor Author

hacki11 commented Sep 28, 2018

Did not have the time yet, but I will let you know when I am finished ;)

fixed review findings
@hacki11
Copy link
Contributor Author

hacki11 commented Oct 3, 2018

Hi Ned, finally added some tests and ready for review.

@nedtwigg
Copy link
Member

nedtwigg commented Oct 3, 2018

Looks great, thanks very much! Merging and releasing.

@nedtwigg nedtwigg merged commit c15371e into diffplug:master Oct 3, 2018
@nedtwigg
Copy link
Member

nedtwigg commented Oct 3, 2018

Published.

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