-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Update documentation for published Maven artifacts #540
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
Conversation
<tr> | ||
<td><code>org.jacoco</code></td> | ||
<td><code>org.jacoco.cli</code></td> | ||
<td><code>nodeps</code></td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof
Answering on #525 (comment) : IMO Command Line Interface
in Maven Repository can be presented as a single artifact with all dependencies included, i.e. replace current artifact without classifier by artifact with classifier nodeps
- it is nor OSGi Bundle, nor provides any APIs and this will be consistent with how it is presented in our ZIP distribution. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin Some arguments to keep nodeps
I can think of:
- This is consistent to the classifier for the Ant module.
- Deploying shaded JARs to Maven is violation of best practice. So we have a explicit warning.
- If CLI would offer a API one day we can publish a un-shaded version without classifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deploying shaded JARs to Maven is violation of best practice. So we have a explicit warning.
While writing first comment I was about to even say: let's don't deploy it. But stopped myself, because it is nice distribution channel. Where you can get it without the need to unpack our all-in-one zip archive. And use it as a dependency to pack it into something else and redistribute. But not as an API dependency, i.e. not for placement into classpath as non-resource. IMO this is the only valid reason for deployment and in such case JAR without classifier is kind of useless, so not sure that in such case statement about "violation" is valid.
If we'll keep deployment, then we can
- relocate classes of dependencies to avoid conflicts if someone will decide to place it into classpath
- relocate all classes into hardly usable package (similarly to what we do for Agent) to prevent usage
- add a big warning on this page and maybe in description in
pom.xml
about usage of this JAR as dependency (maybe we should do this in any case and add similar warning to this page aboutorg.jacoco.agent
JAR withruntime
classifier)
If CLI would offer a API one day we can publish a un-shaded version without classifier.
Anyway we'll be able to do so since prior to this point it is not supposed to be used as API 😉
This is consistent to the classifier for the Ant module.
That's true, however we provide nodeps
to avoid common problem of conflicting dependencies in Ant, so not sure that one without classifier has big popularity. It is used by Gradle Plugin where it is properly isolated, but as far as I can see for no advantage and nodeps
can be used instead.
All in all I'm fine with any option - whether it is removal from deployment, repackaging, or even keeping as is. In last case this PR looks complete, unless we want to add warning(s) about usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re future API I meant to say that in this case we will have two versions:
- A API version without dependencies
- A
nodeps
version
For this situation the non-classifier version should be reserved for case 1.
I would leave documentation as it is, but repackage 3rd party dependencies in a separate PR, created #561 for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof under "repackage" you mean relocate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Godin Sorry, of course "relocate"
In
repo.html
we describe some of the artifacts we deliver to maven. The list should be completed to list all artifacts along with their respective Maven classifiers - see #525 (comment)