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

upgrade bundled maven to 3.8.7 #5170

Merged
merged 1 commit into from
Jan 4, 2023
Merged

upgrade bundled maven to 3.8.7 #5170

merged 1 commit into from
Jan 4, 2023

Conversation

mbien
Copy link
Member

@mbien mbien commented Dec 31, 2022

https://maven.apache.org/docs/3.8.7/release-notes.html

maven dropped the commons-io dependency, other then that everything should stay the same.

some ignored overlaps files might need adjusting, lets see what CI says.

@mbien mbien added Upgrade Library Library (Dependency) Upgrade Maven [ci] enable "build tools" tests ci:all-tests [ci] enable all tests labels Dec 31, 2022
@mbien mbien added this to the NB17 milestone Dec 31, 2022
Comment on lines 18 to +19
5D9CE6ADD7B714B8095F0E3E396C5E9F8C5DCFEF org.apache.maven.shared:maven-dependency-tree:2.2
3ABB90E23BE975D397BBAA43773DD0861DAE26C6 org.apache.maven:apache-maven:3.8.6:bin@zip
4CAAED06DFADFE54EA5C0DD8806EFC0B65812081 org.apache.maven:apache-maven:3.8.7:bin@zip
Copy link
Member Author

Choose a reason for hiding this comment

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

note for later: maven-dependency-tree:2.2 is super old. Probably worth updating in separate PR

Comment on lines 26 to 27
1637B7E8FC392E389752E79B827B883629285626;org.apache.maven:maven-artifact:3.8.7
4D22A3FAA8880EFEF2E960BB8A00C2A0B351C46A;org.apache.maven:maven-builder-support:3.8.7
Copy link
Member Author

@mbien mbien Jan 1, 2023

Choose a reason for hiding this comment

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

btw I haven't updated a single hash in this file and everything is green for some reason. @ebarboni are some CI checks missing or did I forget something important?

verify-libs-and-licenses:
[verifylibsandlicenses] /home/runner/work/netbeans/netbeans/nbbuild/build/verifylibsandlicenses.xml: 0 failures out of 5 tests

BUILD SUCCESSFUL
Total time: 31 seconds

Copy link
Member Author

Choose a reason for hiding this comment

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

updated the hashes now after I noticed there is a manual checkhash target.

The zip is hashed and checked as regular build step since its in the binaries-list. I suppose the binariesembedded-list isn't checked because it would be redundant? I am wondering why it exists, just to list the jars?

Copy link
Contributor

Choose a reason for hiding this comment

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

The file is used to get the maven coodinates for the binaries in the zip. See here:

netbeans/nbbuild/build.xml

Lines 249 to 268 in 6afa3a9

<target name="getallmavencoordinates">
<concat destfile="${nb_all}/nbbuild/build/external.info">
<fileset dir="${nb_all}" includes="**/external/binaries-list" />
<fileset dir="${nb_all}" includes="**/external/binariesembedded-list" />
<filterchain>
<!-- remove comment inline not inline -->
<tokenfilter>
<replaceregex pattern="#(.*)$" replace="" flags="gim" />
</tokenfilter>
<!-- : is sparator for maven coordinate -->
<linecontains>
<contains value=":"/>
</linecontains>
<!-- needed as separator -->
<tokenfilter>
<replacestring from=" " to=";"/>
</tokenfilter>
</filterchain>
</concat>
</target>

That file is then further used by the build when the maven repository is generated.

Copy link
Member Author

@mbien mbien Jan 2, 2023

Choose a reason for hiding this comment

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

yeah I was just wondering if there is any reason to update the hashes for us. If there is: we could hook the check (checkhash) into the build or call it directly from the paperwork job.

If there is no reason to update the hashes since the file is only used as list of jars and the zip is checked anyway, we could just leave a note to not bother with it and only update artifact name and version.

@mbien
Copy link
Member Author

mbien commented Jan 1, 2023

getting some warnings when running maven projects:

--- exec-maven-plugin:3.1.0:exec (default-cli) @ mavenproject1 ---
Hello World!
Failed to notify spy org.netbeans.modules.maven.event.NbEventSpy: org/json/simple/JSONObject
Failed to notify spy org.netbeans.modules.maven.event.NbEventSpy: org/json/simple/JSONObject
Failed to notify spy org.netbeans.modules.maven.event.NbEventSpy: org/json/simple/JSONObject
------------------------------------------------------------------------
BUILD SUCCESS
------------------------------------------------------------------------

this isn't caused by #5111 so it must be this PR

edit:
added --debug:

Failed to notify spy org.netbeans.modules.maven.event.NbEventSpy: org/json/simple/JSONObject
java.lang.NoClassDefFoundError: org/json/simple/JSONObject
    at org.netbeans.modules.maven.event.NbEventSpy.onEvent (NbEventSpy.java:85)
    at org.apache.maven.eventspy.internal.EventSpyDispatcher.onEvent (EventSpyDispatcher.java:104)
    at org.apache.maven.eventspy.internal.EventSpyExecutionListener.projectDiscoveryStarted (EventSpyExecutionListener.java:47)
    at org.apache.maven.lifecycle.internal.DefaultExecutionEventCatapult.fire (DefaultExecutionEventCatapult.java:57)
    at org.apache.maven.lifecycle.internal.DefaultExecutionEventCatapult.fire (DefaultExecutionEventCatapult.java:42)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:217)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:192)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:105)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:960)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:293)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:196)
    at jdk.internal.reflect.DirectMethodHandleAccessor.invoke (DirectMethodHandleAccessor.java:104)
    at java.lang.reflect.Method.invoke (Method.java:578)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:282)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:225)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:406)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:347)

@matthiasblaesing
Copy link
Contributor

@mbien you are seeing a problem in the build process (or better the result of) netbeans-eventspy.jar. The error message indicates, that org.json.simple.JSONObject is not found. But according to the build process, it should not even be there as it is shaded to org.netbeans.shaded.json.simple.JSONObject:

<jarjar jarfile="${cluster}/maven-nblib/netbeans-eventspy.jar" compress="false">
<fileset dir="build/mavenclasses" >
<include name="org/netbeans/modules/maven/event/**"/>
<include name="META-INF/plexus/**"/>
</fileset>
<zipfileset src="../../ide/libs.json_simple/external/json-simple-1.1.1.jar"/>
<rule pattern="org.json.**" result="org.netbeans.shaded.json.@1"/>
</jarjar>

I decompiled the resulting NbEventSpy class and indeed it contained the original reference, not the shaded one. Some more googling showed, that the used jarjar version we use fails to work with JDK 8+ class files. An updated jarjar version fixes that.

Please have a look here: https://github.com/matthiasblaesing/netbeans/tree/pr-5170. And/or here: matthiasblaesing@d9d1309.

Feel free to pull it in.

@mbien mbien added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Jan 1, 2023
@mbien
Copy link
Member Author

mbien commented Jan 2, 2023

@matthiasblaesing oh I get it. I was trying to find where the event spy is supposed to go, found it now:
./nbbuild/netbeans/java/maven-nblib/netbeans-eventspy.jar
I searched in the wrong module for JSONObject. It makes sense now.

Is it ok to squash and add you as co-author?

Maven dist dropped commons-io as dependency.

Fix building of ueber-jar for netbeans-eventspy:

The shading applied by jarjar tool fails on JDK 8+. In consequence the
classes from simple json are renamed, but the references to these
classes are not fixed. This works as long as a copy of simple json is
also on the classpath.

Co-authored-by: Matthias Bläsing <mblaesing@doppel-helix.eu>
@mbien mbien removed the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Jan 2, 2023
@mbien mbien marked this pull request as ready for review January 2, 2023 19:47
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks sane to me and a quick(!) test showed, that it seems to work.

@mbien
Copy link
Member Author

mbien commented Jan 3, 2023

tested it with several larger projects and saw no anomalies. Going to merge it once master is green again.

Copy link
Contributor

@ebarboni ebarboni left a comment

Choose a reason for hiding this comment

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

+1 to merge thanks for doing this. Maven 4 is on the way to not sure on how transition will be easy 😄

@mbien mbien merged commit 3bf285b into apache:master Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests Maven [ci] enable "build tools" tests Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants