-
Notifications
You must be signed in to change notification settings - Fork 9
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
Upgrades gradle wrapper to the latest version #2865
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. The change looks alright (verified it is the same as the one excavator-bot tried to do), but it seems that something might have changed in gradle-sls-packaging or the nebula publishing plugin perhaps:
[00:00:17] FAILURE: Build failed with an exception.
[00:00:17]
[00:00:17] * Where:
[00:00:17] Script '/home/ubuntu/atlasdb-internal-testing/gradle/publish-jars.gradle' line: 4
[00:00:17]
[00:00:17] * What went wrong:
[00:00:17] A problem occurred evaluating script.
[00:00:17] > Failed to apply plugin [id 'com.jfrog.artifactory']
[00:00:17] > Could not create task of type 'ArtifactoryTask'.
[00:00:17]
[00:00:17] * Try:
[00:00:17] Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
[00:00:17]
[00:00:17] BUILD FAILED
[00:00:17]
[00:00:17] Total time: 17.151 secs
@jeremyk-91 I'll own investigating what's going on here. |
There's a dependency conflict somewhere in the buildscript dependencies, relating to guava:
However, I can't run depencencyInsight because I hit the same roadblock! |
cc8f174
to
4236a34
Compare
We don't use it anymore
Downgraded jackson back to 2.6.7 to fix NoClassDefFound errors
This reverts commit 9523808.
Reassigning to @jeremyk-91 for review. However, this PR has high potential for merge conflicts, and there's also the danger that someone will, in the meantime, introduce something that will make the new dependency-recommender version freak out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've sanity checked the changes - looks reasonable, and the new structure of dependencies for QoS and TimeLock is considerably more readable.
Agree that the points under 'future work' should indeed be future work - this is already a fairly large PR.
new GreaterOrEqual<>(lowerBoundInclusive), | ||
new LessThan<>(upperBoundExclusive) | ||
)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
static void sanityCheckTableName(TableReference tableRef) { | ||
String tableName = tableRef.getQualifiedName(); | ||
Validate.isTrue(!(tableName.startsWith("_") && tableName.contains(".")) | ||
|| AtlasDbConstants.hiddenTables.contains(tableRef) | ||
|| tableName.startsWith(AtlasDbConstants.NAMESPACE_PREFIX), "invalid tableName: " + tableName); | ||
|| tableName.startsWith(AtlasDbConstants.NAMESPACE_PREFIX), | ||
"invalid tableName: %s", tableName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, no idea we used commons-lang for these
Started as a copy of #2824... became a massive hunt for broken and outdated dependencies.
Goals: eliminate dependency conflicts across the repo
Implementation description
The root cause of the test failures was that Timelock and QoS each have a special set of dependencies. QoS was simply using Timelock's old, kind of janky dependency specification logic. QoS needs some of the same custom dependencies as Timelock, but not all. This is now made explicit via
qos.props
andtimelock.props
Future work
versions.gradle
- we should unify this with theprops
files so that dependencies are specified in one place.Upgrade baseline/errorprone (we're on v13, v18 is available)PTExecutors.java
; I'll open a separate PR for this after this merges, possibly as part of MMM next week.This change is