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

[MNG-8005] Fix workspace reader drop bug #1385

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Jan 18, 2024

Maven4 seems sets but then drops/overrides some workspace readers. Partially due hazy borders who or what manages them, as role is split between session factory and session factory caller.

Changes:

  • session factory does NOTHING re workspace readers, it becomes fully the caller duty
  • two spots calling session factory (default maven, extension bootstrap) are now fully in charge to properly setup workspace readers

Co-authored-by: Jonas Rutishauser jonas.rutishauser@alumni.ethz.ch


https://issues.apache.org/jira/browse/MNG-8005

Maven4 seems sets but then drops/overrides some workspace
readers. Partially due hazy borders who or what manages
them, as role is split between session factory and session
factory caller.

Changes:
* session factory does NOTHING re workspace readers, it
  becomes fully the caller duty
* two spots calling (default maven, extension bootstrap) are
  now fully in charge to properly setup workspace readers

---

https://issues.apache.org/jira/browse/MNG-8005
@cstamas cstamas requested a review from gnodet January 18, 2024 09:54
@cstamas cstamas self-assigned this Jan 18, 2024
}

public List<CoreExtensionEntry> loadCoreExtensions(
MavenExecutionRequest request, Set<String> providedArtifacts, List<CoreExtension> extensions)
throws Exception {
try (CloseableSession repoSession = repositorySystemSessionFactory
.newRepositorySessionBuilder(request)
.setWorkspaceReader(ideWorkspaceReader)
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't that override the existing chained workspace reader ? is that the intent ?

Copy link
Member Author

Choose a reason for hiding this comment

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

With this PR session factory does NOT set any workspace reader anymore (read PR description and see code), it is caller duty

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I don't question the change or responsibilities, but a possible change in behaviour.
The caller is BootstrapCoreExtensionManager and now responsible to set the workspace reader on the session. What I'm wondering is if this leads back to the same behaviour... I think before this PR, the workspace reader is to set a chained one, now, it's only the ide one...

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, you mean IF request has one as well? Hm, let me recheck is that possible...

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, clearly IS possible (as copy of "origina" request is used), fixing...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

But your comment is wrong: session factory never used chained WSReader, it was ONLY the default maven using it. Before this PR, the session could be inited with request WS (if non null) OR the ide WS (that may be null).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed and also simplified, now chained WS omits possible null entries in collection, thus all these become one liner.

@cstamas cstamas merged commit 3f47580 into apache:master Jan 18, 2024
18 checks passed
@cstamas cstamas deleted the MNG-8005-take-two branch January 18, 2024 11:51
@cstamas cstamas added this to the 4.0.0-alpha-13 milestone Jan 19, 2024
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.

2 participants