scijava / scijava-maven-plugin

A Maven plugin to manage development of SciJava-based software.
BSD 2-Clause "Simplified" License
3 stars 5 forks source link

Improve detection of conflicting artifact versions #21

Closed stelfrich closed 5 months ago

stelfrich commented 6 years ago

Currently, there is a filename/regex-based check for determining if a file in scijava.app.directory conflicts with the artifact to be installed. While this check works, it has hardcoded patterns and is thus not extensible w.r.t. new architectures, etc.

We (@ctrueden and I) have come up with the following scheme to extract the version and qualifier of an artifact:

  1. Look in Jar for version information:
    1. pom.properties in META-INF/maven/<groupId>/<artifactId>/ (if found, return)
    2. pom.xml in META-INF/maven/<groupId>/<artifactId>/ (if found, return)
    3. MANIFEST.mf (Implementation-Version)
  2. If a version is found in the filename suffix, the rest is a qualifier [If not: ij.jar or inconsistent: warning]
  3. If nothing is in the filename
    1. use regex (dash+digit: substring)
    2. if there is no version, assume there is a qualifier or suffix
LauLauThom commented 5 years ago

Example of case when the regex results in having 2 distinct dependency artefacts erroneously overwritten for IJ-OpenCV, namely opencv and opencv-platform.

stelfrich commented 5 years ago

Some results of a recent investigation:


An exemplary pom.properties contains:

#Created by Apache Maven 3.0.5
version=3.4.2-1.4.2
groupId=org.bytedeco.javacpp-presets
artifactId=opencv

The issue with that is that qualifiers are not contained in here.


The pom.xml that is packaged into the JAR files at META-INF/maven/<groupId>/<artifactId>/ is not an Effective POM but still contains variables and some such. Hence, the POMs for different architectures are exactly the same.


The MANIFEST.MF of the native JARs doesn't contain a Implementation-Version.

ctrueden commented 5 years ago

I'd forgotten all about this issue, sorry. For now, let's just add to the regex to cover the IJ-OpenCV use case. @LauLauThom What are the exact filenames that are separate classifiers, and scijava-maven-plugin should detect as such?

The logic to change lives in scijava-common in org.scijava.util.FileUtils. Edit: That logic affects the ImageJ Updater, but not this plugin here. We should find a way to unify the reasoning.

Here is a recent commit after which our commit should be modeled: https://github.com/scijava/scijava-common/commit/32c4098ab3c6aab7efc0a2d09464adbda95d8489

It's too bad the Java ecosystem has not unified around fewer native JAR wrapper naming schemes. But adding to the regex is straightforward—especially if you also add a unit test ensuring it's working as desired.

imagejan commented 4 years ago

We also might need to special-case classifiers such as -swing in miglayout-3.7.4-swing.jar see this discussion on gitter.

rimadoma commented 4 years ago

After I excluded com.miglayout.miglayout from both net.imagej.imagej and net.imagej.imagej-ops I noticed that the issue also affects the artefacts org.scala-lang.modules:scala-xml_2.12:jar:1.0.6 and com.squareup.okhttp3:okhttp:jar:3.6.0 due similarly awkward naming schemes. The former is a dependency of org.scijava:scripting-scala and the latter org.scijava:scijava-io-http.

Furthermore, excluding com.miglayout.miglayout is not really a viable workaround, because it breaks imageJ = new ImageJ(); imagej.launch(), which makes development awkward.

ctrueden commented 4 years ago

@rimadoma We are in the process of switching to the newer miglayout-swing 5.2 which does not use swing as a classifier anymore—see scijava/pom-scijava#97. So this issue of scijava-maven-plugin not working well with miglayout specifically will be moot. I'm working on pom-scijava 29 now, which will include this update.

In the meantime, in addition to excluding the old miglayout artifact, try also adding com.miglayout:miglayout-swing:5.2 as a dependency. Happy to help if you are stuck.

P.S. About your GitHub tagline "L'ordinateur a dit 'Non'": are you quoting Little Britain‽ 😆 🏆

rimadoma commented 4 years ago

@ctrueden Adding com.miglayout:miglayout-swing:5.2 on top of excluding the old does the trick! Thanks very much! The other necessary exclusions I mentioned above don't seem to cause any problems for us.

P.S. I am. Just having fun with my pidgin French. TBH I don't really know the show, but as a programmer that quote speaks to me.

ctrueden commented 4 years ago

@LauLauThom Could you please test pom-scijava 29.0.0-beta-1—which uses scijava-maven-plugin 2.0.0—and report back whether there are still problems with IJ-OpenCV? With cca73a71d5280aeebfb0bb167fc1310f14ae4c7f it might be fixed. And if it's not fixed, I'm guessing that fixing it now will be much more trivial.

LauLauThom commented 4 years ago

@ctrueden Yeah that works ! Thanks a lot ! And sorry for the late reply, yesterday was off 😎

Just the updater recognizes some artifacts as conflicting. But I guess it is relying on the same logic than Maven no ? so could it be just a matter of updating it too ? And I just found out I can specify the OS platform as a maven property to copy only the correct OS dependencies, which actually prevents the error, and for miglayout I think it was mentioned as a work in progress on gitter, right?

image

ctrueden commented 5 months ago

Reviewing this issue now in 2024, I don't see any more work that needs to be done, so... closing! Please reopen if you continue to have issues with conflicting artifact versions not being treated properly by the plugin.