New baseline-class-uniqueness.lock file allows incremental adoption #1145
Conversation
Generate changelog in
|
...seline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineClassUniquenessPlugin.java
Outdated
Show resolved
Hide resolved
...seline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineClassUniquenessPlugin.java
Outdated
Show resolved
Hide resolved
| if (!onDisk.equals(expected)) { | ||
| throw new GradleException(lockFile | ||
| + " is out of date, please run `./gradlew " | ||
| + "checkClassUniquenessLock --write-locks` to update this file"); |
There was a problem hiding this comment.
I think we may want to provide a different message if new duplicates appear vs when problems are fixed, or mention here that it's dangerous to have duplicate classes on the classpath and explain why it's worth spending time to fix.
Otherwise I think folks will see "run X to update locks" and they'll ./X.
iamdanfox
force-pushed
the
lockable-class-uniqueness
branch
from
bd1da4b
to
f34a748
Compare
...seline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineClassUniquenessPlugin.java
Show resolved
Hide resolved
...seline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineClassUniquenessPlugin.java
Show resolved
Hide resolved
| + "cause different behaviour depending on classpath ordering.\n" | ||
| + "# Run ./gradlew checkClassUniqueness --write-locks to update this file\n\n"; | ||
|
|
||
| // not marking this as an Input, because we want to re-run if the *contents* of a configuration changes |
There was a problem hiding this comment.
shouldn't the contents only change if a dependency on the configuration changes?
There was a problem hiding this comment.
Configurations are not Serializable, so I can't just mark a list of them as an @Input.
There was a problem hiding this comment.
you can take the configuration as an input by annotating it with @classpath
There was a problem hiding this comment.
I think that @classpath annotation would work if I had just a single configuration, but in order to preserve the ability to analyze multiple configurations, I don't have just one single configuration here but a bunch of them?
There was a problem hiding this comment.
Instead of storing a set of configurations could you create a single configuration which extends from all of the provided configurations
There was a problem hiding this comment.
I dunno, it seems that has a slight risk of false positives... e.g. if we're analyzing two configurations foo and bar and we move a jar from foo -> bar. In the big clobbered configuration there would be no change, but actually I would expect the lockfile to change.
|
FWIW I've been iterating on this with share-link-service and it nicely highlights all the jakarta problems, then when I apply |
|
Fantastic! |
Before this PR
It's pretty hard to roll out the
baseline-class-uniquenessplugin because it currently is quite an "all or nothing" thing - either you're 100% compliant or else it fails. We haven't really invested in this up until now, because not that much seems to go wrong.HOWEVER - a recent c-j-r dependency upgrade brought in new 'jakarta' jars, which are renamed versions of javax ones (e.g.
javax.ws.rs:javax.ws.rs-api:2.1andjakarta.ws.rs:jakarta.ws.rs-api:2.1.6are pretty much the same thing). People are now starting to get confusing runtime errors from these. Internally, I've got a plugin to automatically fix the problem (gradle-jakarta-renames), but in order to roll that out, I want to ensure people can code-review the changes that it causes.After this PR
==COMMIT_MSG==
Users can now
apply plugin: 'com.palantir.baseline-class-uniqueness'even if they are not yet fully compliant, as the plugin will now just record conflicts in a lock-file. This makes it easier to incrementally improve projects, rather than asking people to fix things in a big-bang way.==COMMIT_MSG==
@raiju for SA who cared about this stuff before
before merging
Possible downsides?