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

Default value must be manually handled #953

Merged
merged 2 commits into from May 22, 2024
Merged

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Apr 20, 2024

Default must be manually handled, i missed this.

Fixes #912

@wendigo
Copy link

wendigo commented Apr 20, 2024

That works :) Thanks!

@cstamas
Copy link
Member Author

cstamas commented Apr 20, 2024

Am adding IT (something I'd done initially would catch this), and will merge

@cstamas
Copy link
Member Author

cstamas commented Apr 20, 2024

The code change is OK and fixes issue, but something else is wrong.
Created #957

The maven.config is ignored by mvnd, hence it tries (local repo shows the attempt) to load extension foo:bar but naturally fails, but continues instead to fail the build.

@wendigo
Copy link

wendigo commented Apr 22, 2024

Can we get this in for alpha14?

@cstamas
Copy link
Member Author

cstamas commented Apr 22, 2024

Sure, this will be in it. But as i wrote, seems there is something else to be fixed as IT this PR adds shows...

@cstamas
Copy link
Member Author

cstamas commented Apr 22, 2024

@gnodet If you run this PR locally:

  • it will pass ok, but
  • maven-conf-ignore-ext IT is wrong: maven.conf where ignore is, is not observed by mvnd. Also local repository will show that extension foo:bar is attempted to resolve, but naturally unsuccessfully
  • this means that mvnd happily goes along instead to fail (as it failed to resolve the extension)

Any idea?

@gnodet
Copy link
Contributor

gnodet commented Apr 22, 2024

@gnodet If you run this PR locally:

  • it will pass ok, but
  • maven-conf-ignore-ext IT is wrong: maven.conf where ignore is, is not observed by mvnd. Also local repository will show that extension foo:bar is attempted to resolve, but naturally unsuccessfully
  • this means that mvnd happily goes along instead to fail (as it failed to resolve the extension)

Any idea?

Yes, the problem is here:

String exclusionsString =
systemProperty(Environment.MVND_CORE_EXTENSIONS_EXCLUDE).asString();
String exclusionsString = systemProperty(Environment.MVND_CORE_EXTENSIONS_EXCLUDE)
.asOptional()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be:

String exclusionsString =
                systemProperty(Environment.MVND_CORE_EXTENSIONS_EXCLUDE).orDefault().asString();

.filter(e -> !exclusions.contains(e.getGroupId() + ":" + e.getArtifactId()))
.collect(Collectors.toList());
if (!exclusions.isEmpty()) {
LOG.info("Excluded extensions (GA): {}", exclusions);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should definitely not be part of the output, and not 4 times.
I'll revert this chunk, or log to debug at most.

@cstamas
Copy link
Member Author

cstamas commented Apr 23, 2024

Unsure why ubuntu failed, win passed.... locally also passed w/ same CLI params as CI invoked 🤔

@@ -224,7 +224,7 @@ public enum Environment {
null,
"io.takari.maven:takari-smart-builder",
OptionType.STRING,
Flags.OPTIONAL),
Flags.DISCRIMINATING | Flags.OPTIONAL),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this one is discriminating. What is discriminating is the list of extensions to load which may actually be affected by this property, but not necessarily. If you add an exclusion which is not supposed to be loaded, this won't affect the result. In addition, that property is not really interesting on the daemon side, as the extensions list is loaded by the client...

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

cstamas added a commit to cstamas/maven-mvnd that referenced this pull request May 22, 2024
Default must be manually handled, i missed this.

Fixes apache#912

Backport of apache#953 to mvnd-1.x
@cstamas cstamas merged commit 6de7431 into apache:master May 22, 2024
4 checks passed
@cstamas cstamas deleted the fix-912 branch May 22, 2024 14:33
cstamas added a commit that referenced this pull request May 22, 2024
Default must be manually handled, i missed this.

Fixes #912

Backport of #953 to mvnd-1.x
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.

Detect smart builder in .mvn/extensions.xml and ignore it
4 participants