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

Split jpms modules into independet artifacts instead of classifier based solution #1317

Merged

Conversation

matthiasblaesing
Copy link
Member

@matthiasblaesing matthiasblaesing commented Feb 20, 2021

The jna-platform artifact depends on jna - this leads to problems when
the JPMS artifacts are used, as the pom.xml for the classifier based
artifacts are identical to the base artifacts. This leads to

  <dependency>
    <groupId>net.java.dev.jna</groupId>
    <artifactId>jna-platform</artifactId>
    <version>5.7.0</version>
    <classifier>jpms</classifier>
  </dependency>

depending on

  <dependency>
    <groupId>net.java.dev.jna</groupId>
    <artifactId>jna</artifactId>
    <version>5.7.0</version>
  </dependency>

so the jna-platform JPMS artifacts pulls in the non-JPMS jna artifact.

To solve this, both artifacts are moved into their own groupId. So

  <dependency>
    <groupId>net.java.dev.jna</groupId>
    <artifactId>jna-platform</artifactId>
    <version>5.8.0</version>
    <classifier>jpms</classifier>
  </dependency>

becomes

  <dependency>
    <groupId>net.java.dev.jna.jpms</groupId>
    <artifactId>jna-platform</artifactId>
    <version>5.8.0</version>
  </dependency>

@matthiasblaesing
Copy link
Member Author

@dbwiddis @neilcsmith-net you two experimented with the JPMS artifacts. What do you think about this change?

@dbwiddis
Copy link
Contributor

I think I prefer it as it's more self-documenting. It's the direction I chose with my project as well, although I also plan to include other Java9+ features.

Not sure why a new groupID is preferred to a new artifactId, but from a downstream implementation perspective it's pretty much the same.

@matthiasblaesing
Copy link
Member Author

@dbwiddis yes I'm still pondering the groupId vs artifactId question (lets paint the bikeshed :-)).

Option 1 (GroupId):

 <dependency>
   <groupId>net.java.dev.jna.jpms</groupId>
   <artifactId>jna</artifactId>
   <version>5.8.0</version>
 </dependency>
  <dependency>
    <groupId>net.java.dev.jna.jpms</groupId>
    <artifactId>jna-platform</artifactId>
    <version>5.8.0</version>
  </dependency>

Option 2 (ArtifactId):

  <dependency>
    <groupId>net.java.dev.jna</groupId>
    <artifactId>jna-jpms</artifactId>
    <version>5.8.0</version>
  </dependency>
  <dependency>
    <groupId>net.java.dev.jna</groupId>
    <artifactId>jna-platform-jpms</artifactId>
    <version>5.8.0</version>
  </dependency>

@dbwiddis
Copy link
Contributor

dbwiddis commented Feb 20, 2021

I like both paint colors in preference to the existing unpainted bike shed for the following reason:

  • I could add a property in my pom.xml to add or remove the -jpms conditionally based on other factors in the build.

I prefer paint color option 2 for the following reason:

  • When I want to see all JNA artifacts at Maven, I would see it right there. With a different groupID I'd have to know it exists to even go looking for it.
    • See for example my project where you instantly get a menu of "oh, a shaded artifact, or a Java11 artifact".
    • See for example Hibernate whose inclusion of separate artifactIds for different java versions was what inspired me to make my choice
    • Or see the same examples and say "that's confusing with so many artifacts, they should be in separate groups.

@dbwiddis
Copy link
Contributor

In the interest of confusing it even more, there's always color 3, making the existing JPMS-compatible MRJAR the primary artifact and letting the few people with a broken build use an older compatible artifact. :-)

@neilcsmith-net
Copy link
Contributor

FWIW, I think the artefact ID is the way to go. I was going to suggest looking at the solution other projects are using, but I'm not keen on the -java11 suffix - -jpms is far more explicit, even if the JDK teams want us all to stop using it. Short but version neutral acronym seems good!

I would leave color 3 until JNA 6.x - then switch to possibly having a -jdk8 or -legacy or -non-jpms option if desired.

@dbwiddis
Copy link
Contributor

I'm not keen on the -java11 suffix - -jpms is far more explicit

Agreed if the only change is the module descriptor. In my case I plan on including other JDK 11+ features so -jpms wasn't broad enough (although I considered and rejected -mrjar).

I would leave color 3 until JNA 6.x

Agree here too!

CHANGES.md Outdated
-----------------
* The maven coordinates of the experimental JPMS (java module system) artifacts
were moved from using the classifier `jpms` to their own group id
`net.java.dev.jna.jpms`, without an classifier. The reason for this is, that
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs updating to match the change to artifactId.

…sed solution

The jna-platform artifact depends on jna - this leads to problems when
the JPMS artifacts are used, as the pom.xml for the classifier based
artifacts are identical to the base artifacts. This leads to

  <dependency>
    <groupId>net.java.dev.jna</groupId>
    <artifactId>jna-platform</artifactId>
    <version>5.7.0</version>
    <classifier>jpms</classifier>
  </dependency>

depending on

  <dependency>
    <groupId>net.java.dev.jna</groupId>
    <artifactId>jna</artifactId>
    <version>5.7.0</version>
  </dependency>

so the jna-platform JPMS artifacts pulls in the non-JPMS jna artifact.

To solve this, the JPMS artifacts are moved to custom artifact ids. So

  <dependency>
    <groupId>net.java.dev.jna</groupId>
    <artifactId>jna-platform</artifactId>
    <version>5.8.0</version>
    <classifier>jpms</classifier>
  </dependency>

becomes

  <dependency>
    <groupId>net.java.dev.jna</groupId>
    <artifactId>jna-platform-jpms</artifactId>
    <version>5.8.0</version>
  </dependency>
@matthiasblaesing
Copy link
Member Author

I squashed the changes, adjusted the wording of the change log and will merge tomorrow if noone objects till then.

@matthiasblaesing matthiasblaesing merged commit acee007 into java-native-access:master Feb 23, 2021
@matthiasblaesing matthiasblaesing deleted the jpms-artifact branch February 23, 2021 20:51
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.

None yet

3 participants