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

Breaking API change in 7.6: MinimalExternalModuleDependency interface adds lots of methods #22701

Closed
yogurtearl opened this issue Nov 12, 2022 · 10 comments
Labels
a:bug affects-version:7.6 closed:not-fixed Indicates the issue was not fixed and is not planned to be in:dependency-version-catalog in:test-suites Work related to the JvmTestSuite Plugin

Comments

@yogurtearl
Copy link
Contributor

starting with 7.6-rc-1, MinimalExternalModuleDependency which is a public interface, now extends ExternalModuleDependency

This adds a lot of methods to the Minimal interface...

This will fail the build or fail at runtime for implementations of the MinimalExternalModuleDependency interface.

Build time error would look like this:

e: /Users/me/src/MyFile.kt: (129, 9): Object 'MinimalExternalModuleDependencyNullObject' is not abstract and 
does not implement abstract member public abstract fun getGroup(): String defined in
 org.gradle.api.artifacts.MinimalExternalModuleDependency

If plugins implement MinimalExternalModuleDependency they will fail at runtime.

Rather than change the public interface MinimalExternalModuleDependency, could you instead make a new NotVeryMinimalExternalModuleDependency ?

@rpalcolea
Copy link
Contributor

Good catch!

I wondering if this falls into the @Incubating contract of things potentially changing until it is stable though hehe

Following this now!

@yogurtearl
Copy link
Contributor Author

MinimalExternalModuleDependency is not marked as @Incubating

And if MinimalExternalModuleDependency has everything in ExternalModuleDependency plus 2 extra functions... Then minimal in the name is misleading. :)

@rpalcolea
Copy link
Contributor

oh you are right! well definitely a breaking change :(

@yogurtearl
Copy link
Contributor Author

Anyone trying to implement a custom impl of the VersionCatalog interface, will need to implement MinimalExternalModuleDependency for this method:

Optional<Provider<MinimalExternalModuleDependency>> findLibrary(String alias);

@yogurtearl
Copy link
Contributor Author

MinimalExternalModuleDependency is marked with @HasInternalProtocol ...

but that is HasInternalProtocol is not visible from the javadoc, so not sure how anyone would know about that unless they read the source code. 🤔
https://docs.gradle.org/7.6-rc-3/javadoc/org/gradle/api/artifacts/MinimalExternalModuleDependency.html

Also, using @HasInternalProtocol on an interface MinimalExternalModuleDependency which is used in VersionCatalog, while VersionCatalog does not have @HasInternalProtocol, is also confusing.

@ljacomet ljacomet added in:dependency-version-catalog in:test-suites Work related to the JvmTestSuite Plugin labels Nov 14, 2022
@eskatos eskatos added 👋 team-triage Issues that need to be triaged by a specific team and removed to-triage labels Nov 23, 2022
@eskatos
Copy link
Member

eskatos commented Nov 23, 2022

Thank you

This issue needs a decision from the team responsible for that area. They have been informed, response time may vary.

@yogurtearl
Copy link
Contributor Author

yogurtearl commented Nov 23, 2022

@eskatos given that this is a breaking API change should it be marked for milestone 7.6, so it can be decided before that release?

IF they decide to fix, it should be fixed before a release. :)

@eskatos eskatos added this to the 7.6 RC5 milestone Nov 23, 2022
@eskatos
Copy link
Member

eskatos commented Nov 23, 2022

Good point, done!

@ljacomet ljacomet added closed:not-fixed Indicates the issue was not fixed and is not planned to be affects-version:7.6 and removed 👋 team-triage Issues that need to be triaged by a specific team labels Nov 24, 2022
@ljacomet ljacomet removed this from the 7.6 RC5 milestone Nov 24, 2022
@ljacomet
Copy link
Member

ljacomet commented Nov 24, 2022

Hi @yogurtearl, we have decided to go ahead and do the 7.6 release with these breaking changes. The main motivation is the new type safe dependency block for test suites, that we plan to make available in more places ... and maybe replace the project level dependencies one at some point.

Ideally, the above changes should have taken in consideration changes to public API interfaces, but that was missed. Designing an evolution instead of a total breaking changes was a difficult challenge, so we will keep this more constrained breakage.

I filed #22848 to improve the situation about types open for extension.

@ljacomet ljacomet closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2022
@yogurtearl
Copy link
Contributor Author

yogurtearl commented Nov 24, 2022

Thanks for the response :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:bug affects-version:7.6 closed:not-fixed Indicates the issue was not fixed and is not planned to be in:dependency-version-catalog in:test-suites Work related to the JvmTestSuite Plugin
Projects
None yet
Development

No branches or pull requests

4 participants