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

#6525: maven execution should setup LegacySupport before running Maven #6552

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Oct 10, 2023

During debugging of #6525, it turned out that during the project read, the execution sometimes avoided the call to setupLegacySupport in our MavenEmebdder before invoking Maven - and when the execution finally reached e.g. NbRepositoryModelResolver.resolveModel, the LegacySupport was already set up by maven core - and by default for online operation.
Futher calls to setupLegacySupport detected an existing session (from Maven) and did not create a NB-compatible setup.

The PR just ensures the LegacySupport is set up prior to maven call, so it is consistent with NB needs.

@sdedic sdedic added the Maven [ci] enable "build tools" tests label Oct 10, 2023
@sdedic sdedic added this to the NB20 milestone Oct 10, 2023
@sdedic sdedic requested review from mbien and dbalek October 10, 2023 13:29
@sdedic sdedic self-assigned this Oct 10, 2023
@sdedic sdedic linked an issue Oct 10, 2023 that may be closed by this pull request
@mbien
Copy link
Member

mbien commented Oct 10, 2023

unfortunately this doesn't solve the issue for me. I am getting the same exception still, following the same steps from the issue. (I use the arrow keys to jump from diff to diff, I could reproduce this twice in a row with fresh config)

@sdedic
Copy link
Member Author

sdedic commented Oct 10, 2023

Heh ... good, I'll give it more rounds of testing. Could you possibly test a diagnostic maven patch ? Someone is creating RepositorySession inside maven...

@mbien
Copy link
Member

mbien commented Oct 11, 2023

Could you possibly test a diagnostic maven patch ?

sure, no problem

@sdedic
Copy link
Member Author

sdedic commented Oct 11, 2023

Hm. So not only unconfigured session. Maven's DefaultLegacySupport contains a thread-local variable that holds the current RepositorySession (through MavenSession). If the "offline" execution happened to be planned on a thread that formerly used "online" embedder, the online session remained in the thread local - and was used instead.

The new commit in this patch replaces the current value with the appropriate one, configured for the executing Embedder; this does not affect other threads (if they use also Maven libraries), as it is replaced in a thread-local variable.

@mbien -- Please test from this PR's branch. There's a Maven patch attached, against maven-3.9.5 tag. As a convenience, I've attached built binaries - to be replaced in java/maven/lib. All that is zipped into the attachment.

maven-patches.zip

@mbien
Copy link
Member

mbien commented Oct 11, 2023

ran a quick test and it appears to be working now (no exceptions), it produces a LOT of log output, so i hope that doesn't influence the timing.
I did what I usually did, opened that one commit with the many poms and went with arrow keys though all of them:
legacy-support.log (outdated)

have no time atm for more, but can test again this evening or tomorrow.

@sdedic sdedic force-pushed the maven/execute-legacy-support branch from 93fded8 to dc0615f Compare October 11, 2023 12:16
@sdedic
Copy link
Member Author

sdedic commented Oct 11, 2023

Do not process the log, my bad - I had one more staged change on my local :( sorry ... replaced the last commit. I set up a value in System properties and check it inside maven libs to ensure that the repository session is 'correct'; the actual fix was already there, but the additional check was broken :)

If you can, try the updated branch in the next time slot -- thanks for your time.

@mbien
Copy link
Member

mbien commented Oct 11, 2023

log is now clean without thread dumps or exceptions

@mbien
Copy link
Member

mbien commented Oct 16, 2023

@sdedic if this is ready to merge, please squash / clean up (remove println) so that we can get this in before freeze.

@sdedic sdedic force-pushed the maven/execute-legacy-support branch from dc0615f to 1aabe20 Compare October 16, 2023 15:53
@sdedic
Copy link
Member Author

sdedic commented Oct 16, 2023

Cleaned up

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

thanks @sdedic!

@mbien mbien merged commit 1fc0a4e into apache:master Oct 16, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Occasional OfflineOperationError when diffing pom files
3 participants