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

apply substitution only to configurations on java SourceSets #10

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

bjlaub
Copy link
Contributor

@bjlaub bjlaub commented Dec 9, 2022

No description provided.

@bjlaub bjlaub requested review from CRogers and carterkozak December 9, 2022 18:53
@changelog-app
Copy link

changelog-app bot commented Dec 9, 2022

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

apply substitution only to configurations on java SourceSets

Check the box to generate changelog(s)

  • Generate changelog entry

@CRogers
Copy link

CRogers commented Dec 9, 2022

👍


public class JakartaPackageAlignmentPlugin implements Plugin<Project> {
@Override
public final void apply(Project project) {
project.getExtensions().getByType(SourceSetContainer.class).configureEach(sourceSet -> {

Choose a reason for hiding this comment

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

perhaps withPlugin to ensure SourceSetContainer exists?

Copy link
Contributor Author

@bjlaub bjlaub Dec 9, 2022

Choose a reason for hiding this comment

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

should we be using withPlugin on java, java-library, or both?

Choose a reason for hiding this comment

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

Is java-base sufficient? I think it's an ancestor of both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and also, looks like java-library will apply java as well, so i think either java or java-base should be sufficient


project.getPluginManager().withPlugin("com.palantir.consistent-versions", _plugin -> {
// necessary to get g-c-v to write substituted dependencies to the versions.lock file
project.getConfigurations().getByName("unifiedClasspath", this::configureConfigurationForSubstitution);

Choose a reason for hiding this comment

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

I wonder if we need something along these lines depending on the order things are applied?

(I defer to @CRogers on this sort of thing, though)

project.getConfigurations().configureEach(new Action<Configuration>() {
    @Override
    public void execute(Configuration config) {
        if ("unifiedClasspath".equals(config.getName())) {
            configureConfigurationForSubstitution(config);
        }
    }
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, i guess? withPlugin guarantees that the action won't run until after the plugin is applied, and this configuration is created when the plugin is applied: https://github.com/palantir/gradle-consistent-versions/blob/develop/src/main/java/com/palantir/gradle/versions/VersionsLockPlugin.java#L220-L226

Comment on lines +37 to +41
project.getPluginManager().withPlugin("com.palantir.consistent-versions", _plugin -> {
// necessary to get g-c-v to write substituted dependencies to the versions.lock file
project.getConfigurations().getByName("unifiedClasspath", this::configureConfigurationForSubstitution);
});
}

Choose a reason for hiding this comment

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

Awareness of specific dependency plugins seems like a bug that may come back to haunt us. I don't know of a better option, but we should make sure we understand why this is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this feels hacky.

I just found this (see: https://docs.gradle.org/current/userguide/resolution_rules.html#sec:dependency_substitution_rules):

Adding a dependency substitution rule to a configuration changes the timing of when that configuration is resolved. Instead of being resolved on first use, the configuration is instead resolved when the task graph is being constructed.

So I wonder if the dependency substitution rules on e.g. runtimeClasspath isn't getting picked up when unifiedClasspath is resolved because the dependencies/constraints are copied to that configuration before the task graph is constructed?

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.

3 participants