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

Grouping of fixes for 7.3.2 #19304

Merged
merged 5 commits into from
Dec 14, 2021
Merged

Conversation

ljacomet
Copy link
Member

Replaces #19297, #19299 and #19301

asodja and others added 5 commits December 13, 2021 20:39
Fix for incremental compilation for classes with $ in the name
No need for a logger implementation for compilation.

Issue #19300
This makes sure a log4j vulnerable version is not available for Zinc
compilation even though it is actually not used by default.

Issue #19300
This constraint makes sure that no vulnerable log4j-core version is made
available on buildscript classpath directly or through plugin
dependencies.

Fixes #19300
@ljacomet ljacomet added this to the 7.3.2 milestone Dec 13, 2021
@ljacomet ljacomet self-assigned this Dec 13, 2021
@bot-gradle
Copy link
Collaborator

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

@bot-gradle
Copy link
Collaborator

Pre-tested commit build failed.

@big-guy
Copy link
Member

big-guy commented Dec 14, 2021

@bot-gradle test and merge

@gradle gradle deleted a comment from ljacomet Dec 14, 2021
@bot-gradle
Copy link
Collaborator

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

Comment on lines +144 to +147
classpathConfiguration.getDependencyConstraints().add(dependencyHandler.getConstraints().create(Log4jBannedVersion.LOG4J2_CORE_COORDINATES, constraint -> constraint.version(version -> {
version.strictly(Log4jBannedVersion.LOG4J2_CORE_STRICT_VERSION_RANGE);
version.prefer(Log4jBannedVersion.LOG4J2_CORE_PREFERRED_VERSION);
})));
Copy link
Member

Choose a reason for hiding this comment

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

Might it be worth forcing an upgrade on API as well? Under this system, you get the following dependencies if strictly was used on API (one of my projects had this):

runtimeClasspath - Runtime classpath of source set 'main'.
+--- org.apache.logging.log4j:log4j-api:{strictly 2.14.1} -> 2.14.1
+--- org.apache.logging.log4j:log4j-core:2.14.1 -> 2.15.0
|    \--- org.apache.logging.log4j:log4j-api:2.15.0 -> 2.14.1
\--- org.apache.logging.log4j:log4j-core:{strictly [2.15, 3[; prefer 2.15.0} -> 2.15.0 (c)

IMO this resolution is already a little weird because it allowed 2.15.0 to be downgraded to 2.14.1, which to my understanding should not occur under a require; but ignoring that: this causes runtime errors. Including this as a default constraint might be surprising for those using strictly on their log4j-api dependency, as it will only fail at runtime!

Copy link
Member Author

@ljacomet ljacomet Dec 14, 2021

Choose a reason for hiding this comment

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

Thanks for sharing, and yes a first level strictly will downgrade a second level require, that is the whole point of the feature.
I wonder if we should bother. Unfortunately, because Log4j does not publish Gradle Module Metadata, it does not benefit from alignment. Though even if it did, this would cause the two strictly to conflict and you would have to add something to the build script.

Note that we are revising the way this constraint is expressed to be in the shape of an reject which is more correct in the long term.

@bot-gradle bot-gradle merged commit cbe27b2 into release Dec 14, 2021
@blindpirate blindpirate deleted the ljacomet/core/release-7.3.2-fixes branch December 14, 2021 02:40
@EwoutH
Copy link

EwoutH commented Dec 15, 2021

Also see the follow up #19311.

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.

None yet

6 participants