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

Guard constraint resolution by a property since it isn't really possi… #353

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

anuraaga
Copy link
Collaborator

…ble to know if a constraint is added by a plugin and out of user control or not.

We found one of the most popular Gradle plugins, the Android plugin, uses constraints. There is one issue in general that if a constraint and dependency both are defined, there's no need to use the constraint in the report (i.e., we want a dependency that is updated to not show in the report even when the constraint is older) which I fix.

But in the end, there isn't a way to know whether a constraint is added by a user or a plugin. I think this also applies for dependencies, but I guess it's less common for plugins to add those. So I've added a flag to enable constraint checking so the user can opt-in to it when they know they can use it.

@ben-manes
Copy link
Owner

Thanks for the speedy fix! Let's get a quirk confirmation from the bug reporters and then release this.

@Vampire
Copy link
Contributor

Vampire commented Oct 15, 2019

I'd like to suggest removing the system property evaluation and only keep the task property.
If someone wants to wire a property to the task property, he can easily do it in his build script.
This would also enable the author of the consuming build to decide whether it should be a project property or a system property and what its name should be.

Additionally I'd like to suggest using @Option for the property, so that automatically a command-line option for the task is supported by gradle. This means you can do gradlew dUp --checkConstraints and the argument has to follow the task and thus the relation is strong and unambiguous.

If a property should remain as default, I'd like to suggest
not using a system property, but a project property and to name it more uniquely to minimise risk for name clashes, for example for a project property com.github.ben-manes.versions.checkConstraints.

@anuraaga
Copy link
Collaborator Author

I personally like project properties too but went with system property since currently all the knobs are system properties. If we want to change we should change all of them but not sure that is in our scope right now.

@Vampire
Copy link
Contributor

Vampire commented Oct 15, 2019

I see, but imho in any way

  • @Option should be used at least additionally so that users with a new enough Gradle version can use it
  • clash-preventing name should be used if a property is used, no matter which kind of
  • optimally no default property

But I guess @ben-manes should comment / decide upon these points

@ben-manes
Copy link
Owner

I am fine either way (including none and only having a task property). This plugin was written in 1.x days so I probably missed some of the best practices and those changed anyway.

I don't think this would be used much at the command line, so I'd lean towards not including it. What do you think about not having it as a command-line option @Vampire, @anuraaga?

@Vampire
Copy link
Contributor

Vampire commented Oct 15, 2019

Well, with @Option it would be cheap, but yeah, this is probably more a configuration of the build and not something someone should decide at runtime.
So I'd agree to neither having a property of any kind, nor a commandline option.

@anuraaga
Copy link
Collaborator Author

Makes sense - removed the property

@ben-manes
Copy link
Owner

Thanks, can you squash and I'll merge?

@anuraaga anuraaga force-pushed the constraints-property branch from 76b4ab3 to 5f4d7d6 Compare October 16, 2019 04:05
@anuraaga
Copy link
Collaborator Author

Oops one sec have to fix README

…ble to know if a constraint is added by a plugin and out of user control or not.
@anuraaga anuraaga force-pushed the constraints-property branch from 5f4d7d6 to 7dba9c4 Compare October 16, 2019 04:06
@anuraaga
Copy link
Collaborator Author

Ok squashed and should be ready

@ben-manes
Copy link
Owner

perfect. I'll let the CI finish, merge, and shortly after release.

@ben-manes ben-manes merged commit 7dba9c4 into ben-manes:master Oct 16, 2019
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