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 to Apache Maven 3.8.4 #3328

Merged
merged 3 commits into from Dec 17, 2021
Merged

Conversation

ebarboni
Copy link
Contributor

As #3239 maven version was broken this is an update to the fixed Maven 3.8.4. Let's have the modern version embedded.

@ebarboni ebarboni added Upgrade Library Library (Dependency) Upgrade Maven [ci] enable "build tools" tests labels Nov 22, 2021
@ebarboni ebarboni added this to the NB13 milestone Nov 22, 2021
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.

Seems to work ok from a quick test (got new dependencies, dependencies could be explored, downloading sources and javadoc worked. I left a tow inline comments.

@@ -32,7 +32,8 @@ ide/db.dataview/external/commons-compress-1.20.jar java/java.disco/external/comm
# It happens that maven 3.3.9 uses commons-lang-2.6.jar, same as o.apache.commons.lang
# and commons-io-2.5.jar, same as java.lsp.server
# These are used independently, different functional goals
java/java.lsp.server/external/commons-io-2.5.jar java/maven.embedder/external/apache-maven-3.6.3-bin.zip
# maven 3.8.4 use common-io-2.6 java/java.lsp.server/external/commons-io-2.5.jar java/maven.embedder/external/apache-maven-3.6.3-bin.zip
platform/o.apache.commons.io/external/commons-io-2.6.jar java/maven.embedder/external/apache-maven-3.8.4-bin.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

The alternative would be to add a dependency on the netbeans module. maven.embedder already seems to depend on commons-logging (this could most probably be removed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure to get the point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maven embedder could use the NetBeans wide installation of commons-io, but this might not even fix the reported error. Anyway - ignore this.

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.

I ran with this the last few days and did not see an issue.

In maven 3.6 the method

ModelBuilder#build(ModelBuildingRequest, ModelBuildingResult)

at the end of the call to

ModelBuilder#build(ModelBuildingRequest)

In 3.8.X the call chain was modified, so a private method of
DefaultModelBuilder is called instead of the two parameter public
method.

The implementation in NBModelBuilder called first the super
implementation. So in essence the sequence in the past was:

- NBModelBuilder#build(ModelBuildingRequest)
  - DefaultModelBuilder#build(ModelBuildingRequest)
    - NBModelBuilder#build(ModelBuildingRequest, ModelBuildingResult)
      - DefaultModelBuilder#build(ModelBuildingRequest, ModelBuildingResult)
      - [NB Extensions]
  - [NB Extensions]

The modified implementation ensures, that the post processing happens
independend from the call chain.
@matthiasblaesing
Copy link
Contributor

Ok - this can't be merged yet, as the unittests for maven fail reproducible (also locallly reproducible!). The issue can be tracked down into a behavioral change in the ModelBuilder implementation of maven. NetBeans overrides a method in the ModelBuilder to enrich the metadata generated. This method was called in 3.6.X from both possible call flows, but is now not called by the default flow anymore. Please have a look at this implementation:

matthiasblaesing@b5b45e3

I suggest to get that into this PR and see what the build will do (locally unittests for maven.embedder and maven ran successfully).

@ebarboni
Copy link
Contributor Author

I will merge this today. Seems unit test on maven is broken but not PR related.

@JaroslavTulach
Copy link

Up to you. However the run says: "ant $OPTS -f java/maven test" exited with 1. Are you sure it is not Maven related ;-?

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.

I point to #3328 (comment): This update as is breaks maven functionality reproducibly. That needs a fix first.

@ebarboni
Copy link
Contributor Author

sorry just show other pending PR having the same ant $OPTS -f java/maven test" exited with 1 on java/maven.
for me this fail on master too.

@ebarboni
Copy link
Contributor Author

@matthiasblaesing I'm sorry but I'm not sure how to add your work on Model into this PR.

I guess 3.8.4 introduce
[junit] Test org.netbeans.modules.maven.execute.ReactorCheckerTest FAILED
[junit] Test org.netbeans.modules.maven.problems.PrimingActionTest FAILED
[junit] Test org.netbeans.modules.maven.problems.ProblemReporterImplTest FAILED

On master we still have that
[junit] Test org.netbeans.modules.maven.runjar.RunJarStartupArgsTest FAILED

@matthiasblaesing
Copy link
Contributor

@ebarboni when in doubt I do this:

  • run on master get all failing tests there
  • rerun tests with my changes
  • if the same tests fail, the tests are broken, if new show up, the changes break something

There is at least one flaky test in maven and I think two, that fail for me locally and work ok in tests. For the difference test, that does not matter much though.

---------- RunJarStartupArgsTest ----------

Without maven.compiler.source and maven.compiler.target set, they are
defaulted to 1.5. This will fail on recent JDKs. Instead use the java
version from the running JDK, which can be assumed to be a save default.

---------- ShellConstructorTest ---------

The test is flaky and it was found, that sometimes the maven CLI version
is reported to be NULL. Closed analysis showed, that this is caused by a
IOException, referencing to an unexpected end of zlib input stream.

The issue seems to be, that the ArchiveURLMapper caches jars and the
test in rapid succession deletes jars and recreates them.

This change moves dummy JAR creation from per Test to per Testcase.
This shoud stabilize the test.
@matthiasblaesing
Copy link
Contributor

I had a look at the tests, that either failed locally (RunJarStartupArgsTest) or are flaky (ShellConstructorTest): matthiasblaesing@d4b4f5f

To get them into this PR, pull the changes like this:

# Ensure the branch backing this PR is checked out
git checkout maven384
# Pull in changes from my branch
git pull https://github.com/matthiasblaesing/netbeans pr-3328
# Push to github
git push

@ebarboni
Copy link
Contributor Author

thanks a lot @matthiasblaesing . except the windows check seems ok now.

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.

Reran tests locally with this branch and all is green and tavis and github agree to. I think this is good to go.

@matthiasblaesing matthiasblaesing merged commit 6432134 into apache:master Dec 17, 2021
@ebarboni ebarboni deleted the maven384 branch July 15, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants