Update to use the new configuration cache safe GradleOperatingSystem

[FinlayRJW]

Before this PR

palantir/gradle-baseline#3140 was being blocked by PJF using OperatingSystem to run commands during the configuration phase via process builder. This break configuration cache and prevents merging.

After this PR

palantir/gradle-utils#383 resolved this by introducing GradleOperatingSystem a configuration cache safe way of getting the operating system, this PR updates PJF to use the new GradleOperatingSystem class fixing the issues allowing for baseline to merge

==COMMIT_MSG==
Update to use the new configuration cache safe GradleOperatingSystem
==COMMIT_MSG==

Possible downsides?

[changelog-app]

Generate changelog in changelog/@unreleased

Type (Select exactly one)

• Feature (Adding new functionality)
• Improvement (Improving existing functionality)
• Fix (Fixing an issue with existing functionality)
• Break (Creating a new major version by breaking public APIs)
• Deprecation (Removing functionality in a non-breaking way)
• Migration (Automatically moving data/functionality to a new system)

Description

Update to use the new configuration cache safe GradleOperatingSystem

Check the box to generate changelog(s)

• Generate changelog entry

[kelvinou01]

Nice! Removing merge when ready so someone else can take a look at this before we merge

Comment was edited

[felixdesouza]

we should just call operatingSystem inside here, it's a field.

[felixdesouza]

hmm, curious whether this get is avoidable where we are?

[FinlayRJW]

I guess we could return a Provider<Boolean> but would we not have to realise that immediately above when we do:

!isNativeImageConfigured(rootProject, operatingSystem))

[FinlayRJW]

I guess you could do:

isNativeImageConfigured(rootProject, operatingSystem).map(isConfigured -> {
            if (!isConfigured) {
                log.info("Skipping native image configuration as it is not supported on this platform");
                return null;
            }
....

but is that any better?

[felixdesouza]

you're realising the operating system thing here, meaning it will always get called even if you're not going to resolve this configuration. You should use getFrom().attributeProvider(...)

[FinlayRJW]

updated to:

attributeProvider(
                            ArtifactTypeDefinition.ARTIFACT_TYPE_ATTRIBUTE,
                            operatingSystem.map(NativeImageFormatProviderPlugin::getExtension));

[felixdesouza]

I kinda hate having to pull this here, just to pass it in.

I have a slightly crazier idea that I think works a bit better:

Have a SpotlessInterop implement Action<JavaExtension>.

public abstract class SpotlessInterop implements Action<JavaExtension> {

  @Nested
  protected abstract GradleOperatingSystem getOs();

  @Inject
  protected abstract ConfigurationContainer getConfigurations();

  public SpotlessInterop() {}

  public void execute(JavaExtension java) {
    java.addStep(spotlessJavaFormatStep());
  }

  private FormatterStep spotlessJavaFormatStep() {
    // same as before but just use the methods instead of fields
  } 
}

Usage then becomes:

SpotlessExtension spotlessExtension = project.getExtensions().getByType(SpotlessExtension.class);
// get from root project to have the right configurations
SpotlessInterop spotlessInterop = project.getRootProject().getObjects().newInstance(SpotlessInterop.class);
project.getPluginManager().withPlugin("java", _plugin -> {
  SPOTLESS_PLUGINS.forEach(
    spotlessPluginId -> project.getPluginManager().withPlugin(spotlessPluginId, _spotlessPlugin -> {
      spotlessExtension.java(spotlessInterop);
    }));
});

Benefits being it's relatively self contained,

[FinlayRJW]

I've converted to this format but needed to keep the project around to get the JavaFormatExtension and run isNativeImageConfigured - on the whole I do think it is neater though:

public abstract class SpotlessInterop implements Action<JavaExtension> {
    private static final Logger logger = Logging.getLogger(SpotlessInterop.class);

    private final Project project;

    @Nested
    protected abstract GradleOperatingSystem getOs();

    @Inject
    protected abstract ConfigurationContainer getConfigurations();

    @Inject
    public SpotlessInterop(Project project) {
        this.project = project;
    }

    @Override
    public void execute(JavaExtension java) {
        java.addStep(spotlessJavaFormatStep());
    }

    private FormatterStep spotlessJavaFormatStep() {...}

[kelvinou01]

Nice! Excited to see this go in.

[CRogers]

As discussed in person, we can make a @Nested NativeImageSupport type that contains these methods along with a @Nested GradleOperatingSystem and @Inject ProviderFactory to avoid having to pass this provider around everywhere

[FinlayRJW]

so I've done this and made a NativeImageSupport type - currently it has a method boolean isNativeImageConfigured would it be better to make the is a Provider<Boolean> or does it not really matter?

[CRogers]

getNativeImageSupport

[felixdesouza]

somehow my comment disappeared, or I never submitted, it, but similar thing here, task.getNativeImage().fileProvider() can take a provider

[felixdesouza]

so very similar to the attributeProvider thing you did

[FinlayRJW]

I'm not sure if I've miss-understood this but getNativeImageConfigured().isNativeImageConfigured() returns a Boolean and not a Provider so I guess you could do something like:

                task.getNativeImage()
                        .fileProvider(getNativeImplConfiguration(project).map(file -> {
                            if (getNativeImageSupport().isNativeImageConfigured()) {
                                return file;
                            }
                            return null;
                        }));

but is that any better?

[FinlayRJW]

actually that doesnt seem to work I get

Could not determine the dependencies of task ':formatDiff'.
> Failed to query the value of task ':formatDiff' property 'nativeImage'.
   > Could not resolve all files for configuration ':palantirJavaFormatNative'.
      > Cannot resolve external dependency com.palantir.javaformat:palantir-java-format-native:null because no repositories are defined.
        Required by:
            root project :

in the test

[felixdesouza]

👍

[CRogers]

👍

[felixdesouza]

I think there are probably nicer ways to get this in i.e. using attributes and module metadata, so we can really just resolve / run programs at the last possible second. I might throw something up at a later point, but in its current form it's pretty hard to avoid

[autorelease3]

Released 2.70.0