gosu-lang / gradle-gosu-plugin

BSD 3-Clause "New" or "Revised" License
5 stars 7 forks source link

Remove more internal APIs #44

Closed wolfs closed 6 years ago

wolfs commented 6 years ago

See https://github.com/gosu-lang/gradle-gosu-plugin/issues/40.

There is no need to use Cast, DefaultSourceSetOutput or LazyInitializedFileCollection. See the PR how this is fixed.

For having the same behaviour as source sets from Gradle itself, SourceDirectorySetFactory needs to be used, so there is currently no way around doing so.

Note that everything in org.gradle.util is also considered internal API, so there are some other things which would need changing. Most of the things in org.gradle.util can be replaced by using a different third party library.

There is also ConfigureUtil.configure: This can be replaced by not having the corresponding method at all and using ObjectFactory. E.g., instead of having

  public GosuSourceSet gosu( Closure configureClosure ) {
    ConfigureUtil.configure(configureClosure, getGosu());
    return this;
  }

  public GosuSourceSet gosu( Action<? super SourceDirectorySet> configureAction) {
    configureAction.execute(getGosu());
    return this;
  }

in DefaultGosuSourceSet you should create an instance of DefaultGosuSourceSet by calling ObjectFactory.newInstance(DefaultGosuSourceSet.class, ...) and then only having the method taking an action:

  public GosuSourceSet gosu( Action<? super SourceDirectorySet> configureAction) {
    configureAction.execute(getGosu());
    return this;
  }

The method accepting a Closure will then be generated automatically - no need for using ConfigureUtil.

DPUkyle commented 6 years ago

Thanks @wolfs this is wonderful. I'm very grateful for your assistance.

My CI process did pick up one regression when executing tests with Gradle 4.0.2. It seems related to o.g.a.internal.file.AbstractFileResolver delegating to ErrorHandlingNotationParser, but I can't quite figure out why.

Since this is so easy to reproduce, could I ask for you opinion on what might be causing this?

Steps to reproduce locally:

  1. $ export CI=true
  2. Shorten gradle.properties:3 to now become testedVersions = 4.0.2
  3. $ ./gradlew test --tests=org.gosulang.gradle.functional.SimpleGosuBuildTest

I did not find anything appearing relevant in the known issues of Gradle 4, 4.0.1, 4.0.2 or 4.1 release notes. Oddly this is fine in 2.12+, 3.x and 4.1+.

In the worst-case, removing support for 4.0.x is a very small price to pay to ensure future compatibility thanks to your help to remove the remaining internal APIs!

wolfs commented 6 years ago

@DPUkyle Seems like we are not resolving Providers in 4.0.2... I think dropping support for 4.0.x would be the best. WDYT?

DPUkyle commented 6 years ago

Thanks again Stefan! Yes I agree, removing support for 4.0.x is acceptable.

One last note to close the loop: Regarding your suggestion to use ObjectFactory#newInstance to instantiate DefaultGosuSourceSet, I gave it a try locally but method newInstance was introduced in Gradle 4.2. So while I like the idea I'm yet comfortable to break backwards compatibility.

Thank you again for your help! I'm very appreciative!

wolfs commented 6 years ago

Does this mean that this PR is ready to merge? Or is there something missing?

DPUkyle commented 6 years ago

Sorry for the delay - I was doing some integration testing with enterprise-level consumers of the plugin.