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

[MRELEASE-1087] Upgrade Maven to 3.2.5 (and de-plexus) #118

Merged
merged 10 commits into from
May 6, 2022

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Apr 29, 2022

Mass changes to plugin:

  • update maven 3.0 -> 3.2.5
  • update resolver from org.sonatype to org.eclipse package
  • off plexus XML, move fully to JSR330
  • off plexus APIs like LogEnabled, Contextualize, etc
  • use slf4j API for logging instead of Plexus Logger
  • tests: off from Junit3 PlexusTestCase w/ XMLs to modern(er) Junit 4
  • tests: off from PlexusTestCase XMLs (one exception remains)
  • reformat

Two UTs are Ignored for now

All but two UTs pass that I cannot interpret:

if ( !"3.0".equals( mavenVersion ) )
{
// bug??
checkVersionLessThanVersion( "1.01-beta-04-20051112.134500-2", "1.01-beta-04-SNAPSHOT" );
}

https://github.com/apache/maven-release/blob/85d973c87724aa2bc0698e6773dd9ce2a0fc8701/maven-release-manager/src/test/java/org/apache/maven/shared/release/phase/GenerateReleasePomsPhaseTest.java

Former currently does not even run (maven version != 3.0), while latter is most probably due bad diff, probably diff logic in UT needs more "no change" detection.

All IT pass OK:

[INFO] --- maven-invoker-plugin:3.2.2:verify (integration-test-prepare) @ maven-release-plugin ---
[INFO] -------------------------------------------------
[INFO] Build Summary:
[INFO]   Passed: 46, Failed: 0, Errors: 0, Skipped: 0
[INFO] -------------------------------------------------

This plugin was so deeply tied to plexus
"magic", not only tests, but also runtime!
@cstamas cstamas self-assigned this Apr 29, 2022
the GenerateReleasePomsPhaseTest
While we figure out what is with this UT specifically.
@cstamas
Copy link
Member Author

cstamas commented May 2, 2022

I am aware this PR is really BIG and nothing else describes it fully than vague "catch up", but we need to perform this jump, as current master is really lagging and IMHO has way too much debt than would be somewhat IMHO acceptable.

@cstamas
Copy link
Member Author

cstamas commented May 2, 2022

There is one problem: all UT and IT pass OK except this one that is disabled (hence build is green): GenerateReleasePomsPhaseTest

I have hard time even to understand what this UT is about, and even less clues WHY it fails, seems it has to do how POM is persisted and some change probably between 3.0 and 3.2 "ways of doing it". So here I'd expect some explanation and help from @hboutemy @rfscholte and @michael-o to figure it out.

@michael-o
Copy link
Member

Will try to go through today/tomorrow.

@hboutemy
Copy link
Member

hboutemy commented May 4, 2022

we need a MRELEASE Jira issue with an update of every commit message to be able to track the change in the future

@slawekjaranowski
Copy link
Member

maven-model-builder/src/main/resources/org/apache/maven/model/pom-4.0.0.xml was changed so we need change expected-*-pom.xml for UT.

Comment on lines 217 to 222
// FIXME ... setDependencyArtifacts - set direct dependencies ...
// but in org.apache.maven.shared.release.phase.GenerateReleasePomsPhase.createReleaseDependencies
// getArtifacts is used ...

// so we probably also need call setResolvedArtifacts

Copy link
Member

Choose a reason for hiding this comment

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

Probable cause of ignore tests in AbstractRewritingReleasePhaseTestCase

Copy link
Member Author

Choose a reason for hiding this comment

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

For example, I have hard time to understand why release needs to resolve anything at all...

Copy link
Member

Choose a reason for hiding this comment

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

And why we need use setDependencyArtifacts directly ...
code is also strange for me we check:

if ( project.getDependencyArtifacts() == null ) 

and next on different object

resolvedProject.setDependencyArtifacts(....)

I will check what happens if I call

resolvedProject.setResolvedArtifacts( resolvedProject.getDependencyArtifacts() )

of course I don't understand it very well

@hboutemy
Copy link
Member

hboutemy commented May 5, 2022

Jira issue created https://issues.apache.org/jira/browse/MRELEASE-1087

@cstamas cstamas changed the title Catch up [MRELEASE-1087] Catch up May 5, 2022
@cstamas
Copy link
Member Author

cstamas commented May 5, 2022

I updated/fixed things reported so far, @slawekjaranowski did re-enable the failing UT GenerateReleasePomsPhaseTest (while he did disable several tests in it's parent class), but also there is that DefaultVersionInfoTest as well...

So, I'd like to see some consensus about these changes and finally make this PR merged.
But please note, that if there are some other pending PRs for upcoming release, better would be to merge those and update this PR, as am pretty sure this PR will render all existing PRs as "conflicted".

@cstamas
Copy link
Member Author

cstamas commented May 5, 2022

Cool, thanks @slawekjaranowski !

@cstamas
Copy link
Member Author

cstamas commented May 5, 2022

@rfscholte or @hboutemy would be good if any of you would bless this PR so we can merge. Again, as before, if there is any other PR scheduled, please merge it BEFORE this one, I will handle conflicts.

But, this PR clearly shows severe issues with m-release-p:

  • why does release redo Maven CLI? (see use of commons-cli), this is obviously a duplication of logic here
  • why does release resolve anything at all? Several of us have hard time to understand what is happening here

All in all, I still think what I said before: "this plugin does WAAY more than it should", and hence, is very fragile (the plugin but it's process as well).

Anyway, let's move on, and we can tinker more about these later.

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

some nit comments/question

@olamy olamy changed the title [MRELEASE-1087] Catch up [MRELEASE-1087] Upgrade Maven to 3.2.5 (and de-plexus) May 6, 2022
* aligned logger uses (using pattern from maven core MNG-7278)
* fixed Arrays FQCN
@cstamas cstamas requested a review from olamy May 6, 2022 06:26
Copy link
Member

@hboutemy hboutemy left a comment

Choose a reason for hiding this comment

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

LGTM: a good pass at cleanup that will prepare for next ones
I hope M6 will be the last milestone and we can after go to final

@michael-o
Copy link
Member

LGTM: a good pass at cleanup that will prepare for next ones I hope M6 will be the last milestone and we can after go to final

I assume that SCM 2 will be in MRELEASE 4 only, no?

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

ship it!

@cstamas cstamas merged commit dafdd7f into master May 6, 2022
@cstamas cstamas deleted the migrate-plugin-off-plexus branch May 6, 2022 07:17
@olamy olamy added the dependencies Pull requests that update a dependency file label May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
6 participants