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

Support constraint without version in GMM #20188

Merged

Conversation

ljacomet
Copy link
Member

No description provided.

Copy link
Member

@octylFractal octylFractal left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor change I had a question about but it's not blocking.

@@ -439,7 +439,7 @@ private IvyArtifactName consumeArtifactSelector(JsonReader reader) throws IOExce
String group = null;
String module = null;
String reason = null;
VersionConstraint version = null;
VersionConstraint version = DefaultImmutableVersionConstraint.of();
Copy link
Member

Choose a reason for hiding this comment

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

❔ - As far as I understand the issue, the fix boils down to this line here.
According to the issue, it was expected that a constraint always have a version.

Copy link
Member Author

@ljacomet ljacomet Mar 17, 2022

Choose a reason for hiding this comment

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

Indeed, but the assumption made here was wrong. The Gradle DSL allows to define a constraint without version. The whole dependency graph engine works fine with a constraint that has an empty version. And there are limited use cases for this - see the added test.

What is clearly not supported by the engine is a null version, this is fixed by this change.

Prior to this change, a published Gradle Module Metadata that contained
a constraint without a version definition would result in a null
VersionConstraint instead of an empty one.
This caused issue during metadata serialization and potentially
resolution.
The field is not nullable and an empty version constraint is now used as
the default value.

Fixes #20182
@ljacomet ljacomet force-pushed the ljacomet/dependency-management/constraint-no-version branch from 8a20db0 to bec7996 Compare March 17, 2022 10:00
@gradle gradle deleted a comment from ljacomet Mar 17, 2022
@gradle gradle deleted a comment from ljacomet Mar 17, 2022
@bot-gradle
Copy link
Collaborator

OK, I've already triggered a build for you.

@blindpirate
Copy link
Collaborator

So the ~10% performance regression seems to be true...

@bot-gradle
Copy link
Collaborator

Pre-tested commit build failed.

@blindpirate
Copy link
Collaborator

@bot-gradle test and merge

@gradle gradle deleted a comment from blindpirate Mar 20, 2022
@bot-gradle
Copy link
Collaborator

OK, I've already triggered a build for you.

@bot-gradle bot-gradle merged commit 8b126e1 into master Mar 20, 2022
@blindpirate blindpirate deleted the ljacomet/dependency-management/constraint-no-version branch March 21, 2022 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Imported BOM parsing fails if published with Gradle Module Metadata
5 participants