-
Notifications
You must be signed in to change notification settings - Fork 597
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
Additional Dependency updates #9006
Conversation
Github actions tests reported job failures from actions build 11391204694
|
0c75f43
to
8d37d17
Compare
Github actions tests reported job failures from actions build 11408724818
|
Github actions tests reported job failures from actions build 11409067427
|
* Update depenencies to fix vulnerabilities as reported in #8950 * Update our dependency management to make use of some newish gradle features * Add dependency constraints to update transitive dependencies, this allows us to specify versions without making them direct dependencies * Remove most force expressions and replace them where necessary with version strict requirements * Make use of several published bom's to configure consistent dependency versions for platforms like netty and log4j2 * Remove exclude statements that are now handled by variant dependency resolution (like guava android vs jdk) * Exclude the org.bouncycastle:bcprov-jdk15on dependency and replace it with bcprov-jdk18onA This adds an unecessary implementation level dependency on what is really a transitive, but I couldn't get gradles explicit replacement logic to work so this is a workaround
8f25d8f
to
4415c71
Compare
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.
Back to you @lbergelson -- minor comments only, looks fine to me overall. Merge when ready!
build.gradle
Outdated
// include the apache commons-logging bridge that matches the log4j version we use so | ||
// messages that originate with dependencies that use commons-logging (such as jexl) | ||
// are routed to log4j | ||
implementation 'org.apache.logging.log4j:log4j-jcl:' + log4j2Version | ||
implementation 'org.apache.logging.log4j:log4j-jcl:' |
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.
stray colon at the end here?
implementation('org.objenesis:objenesis:1.2') | ||
testImplementation('org.objenesis:objenesis:2.1') | ||
implementation 'org.objenesis:objenesis:1.2' | ||
testImplementation 'org.objenesis:objenesis:2.1' |
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.
why is there a different version of this dependency for tests?
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 don't know.... It's always been that way. It's a very good question but I'm not going to figure it out in this PR.
build.gradle
Outdated
implementation('com.intel.gkl:gkl:0.8.11') { | ||
exclude module: 'htsjdk' | ||
} | ||
implementation 'com.intel.gkl:gkl:0.8.11' |
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.
should the GKL version be a named constant up at the top?
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.
yes
build.gradle
Outdated
implementation 'dnsjava:dnsjava:3.6.0' | ||
implementation 'org.apache.commons:commons-compress:1.26.0' | ||
implementation 'org.apache.ivy:ivy:2.5.2' | ||
implementation 'org.apache.commons:commons-text:1.10.0' |
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.
link to the vulnerability report for this one, since we had it in a now-deleted comment:
https://nvd.nist.gov/vuln/detail/CVE-2022-42889
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.
Ok. I dropped it because everything in this block has some variant of "there was a nasty bug but it's fixed in this version". Added it back in.
build.gradle
Outdated
|
||
//this is a replacement for the transitive dependency bcprov-jdk15on:1.70.0 which | ||
//is excluded for security purposes | ||
//this causes this to act as direct dependency of ours but we don't actually rely on it except as a transitive |
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.
document which direct dependency of ours this is a transitive dependency of
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.
Oh wow, I'm glad you asked this. It's a minicluster dependency, so it can actually be a testUtilsImplementation dependency instead.
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 guess the dependency scanner isn't aware the fact that testUtils is really a test configuration since it's not a standard one.
This reverts commit d056c32.
Additional changes on top of #8998