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

Switch to Maven Artifact Resolver Ant tasks #3923

Merged
merged 6 commits into from
Jan 12, 2023

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Dec 9, 2022

The ongoing effort to support Java 17 (#3815) in Bio-Formats highlighted issues with the Maven Ant Tasks component which is used by the Ant build system to retrieve the dependencies from the POM files. More specifically, the upgrade to org.openmicroscopy:ome-codecs:0.4.0 to consume ome/ome-codecs#23 fails with the following stack trace:

[artifact:dependencies] Downloading: io/airlift/aircompressor/0.21/aircompressor-0.21.pom from repository central at https://repo.maven.apache.org/maven2
[artifact:dependencies] Transferring 6K from central
[artifact:dependencies] Downloading: io/airlift/airbase/112/airbase-112.pom from repository central at https://repo.maven.apache.org/maven2
[artifact:dependencies] Transferring 67K from central
[artifact:dependencies] Downloading: org/junit/junit-bom/5.8.0-M1/junit-bom-5.8.0-M1.pom from repository central at http://repo1.maven.org/maven2
[artifact:dependencies] Error transferring file: Server returned HTTP response code: 501 for URL: http://repo1.maven.org/maven2/org/junit/junit-bom/5.8.0-M1/junit-bom-5.8.0-M1.pom
Warning: :dependencies] [WARNING] Unable to get resource 'org.junit:junit-bom:pom:5.8.0-M1' from repository central (http://repo1.maven.org/maven2): Error transferring file: Server returned HTTP response code: 501 for URL: http://repo1.maven.org/maven2/org/junit/junit-bom/5.8.0-M1/junit-bom-5.8.0-M1.pom

The Maven Ant Tasks component has been retired in 2014 and it's almost unexpected that we have been able to use it for so long without issues. This leaves two options to resolve this build problem:

Although retiring the Ant build system comes up periodically in discussions with @ome/formats team, my primary concern is that it is still not properly scoped but we know retiring it will impact developers, CI and release process minimally. While for several components there is a large overlap of the Ant tasks with the standard Maven lifecycle, there are a number of critical tasks which are currently only defined in Ant including the generation of various archives/bundles (bftools, bfmatlab) and the running of the automated tests.

This PR investigates the first option to modernize the Ant dependency management using Maven-compatible tools and allow us to deliver Java 17 support.

Overall, the changes should be straightforward:

  • the maven-ant-tasks is replaced by the maven-resolver-ant-tasks uberJAR and declared in the appropriate Ant configuration files
  • the mvn-init task that was using artifacts:dependencies to create compile/test/runtime classpaths is retired and resolver:resolve is directly involved in the compile and compile-tests tasks. The compile.classpath, test.classpath and runtime.classpath` references should remain unchanged
  • artifact:install is retired in favor of resolver:artifacts and resolver:install
  • the bundle creation is done by first unzipping the runtime.classpath in a temporary build directory and then rezipping it using zipfileset, primarily for performance reasons

sbesson and others added 5 commits December 9, 2022 10:12
- declare new resolver namespace and point to the new taskdef
- remove the maven.repo.local handling (supported by default)
- declare the minimum set of remote repositories to compile
- drop the mvn-init task and include resolve in the relevant tasks
- declare top-level install-pom task to use local pom-bio-formats
This is a requirements for components liked turbojpeg which have
no dependencies as the javac tasks fails with some form of NPE when
fetching the classpath refid
Uses archives, zips and path with the classpath reference. However
it is very slow and can probably be optimized
This is primarily for performance reasons as the previous command
acting on the classpath refid was very slow - see also
https://bz.apache.org/bugzilla/show_bug.cgi?id=43144
@dgault dgault added this to the 6.12.0 milestone Dec 12, 2022
@dgault
Copy link
Member

dgault commented Dec 13, 2022

The overall approach looks sensible to me, happy to see it removed from draft and included in the CI

@melissalinkert
Copy link
Member

Quick test of ant clean jars tools, showinf with the resulting artifacts, and ant ... test-automated on a directory with many datasets seems fine. I don't have any immediate concerns, so fine with taking this out of draft for inclusion in the merge builds.

@sbesson sbesson marked this pull request as ready for review December 13, 2022 21:28
@sbesson
Copy link
Member Author

sbesson commented Dec 14, 2022

One failure in the daily CI build - see https://merge-ci.openmicroscopy.org/jenkins/job/BIOFORMATS-image/1394/console

BUILD FAILED
/bio-formats-build/bioformats/ant/toplevel.xml:288: The following error occurred while executing this line:
/bio-formats-build/bioformats/ant/java.xml:48: Could not resolve artifacts: Could not find artifact org.kohsuke:file-leak-detector:jar:jar-with-dependencies:1.15 in central (https://repo1.maven.org/maven2/)

It's effectively a side-product of the interaction of this PR and #3815 where file-leak-detector is bumped to the next version, requiring the addition of another remote repository in bf047fb.
f06d32a should pre-emptively address this scenario by declaring the repository in ant/java.xml.

Copy link
Member

@dgault dgault left a comment

Choose a reason for hiding this comment

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

All the changes here look to be sensible and the PR has been included as part of the CI for a while now without issues. I manually tested each of the main ant targets and the results were as expected. All the generated artefacts match those produced without the PR and tests all run without issue. PR is good to merge from my side.

@dgault dgault merged commit 60bc356 into ome:develop Jan 12, 2023
@sbesson sbesson deleted the maven-artifact-resolver branch January 12, 2023 16:23
@sbesson sbesson mentioned this pull request Jan 12, 2023
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.

3 participants