-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Tidy up core's build file #4207
Conversation
# Conflicts: # core/build.gradle
core/build.gradle
Outdated
shaded 'org.zeroturnaround:zt-exec:1.12', { | ||
exclude(group: 'org.slf4j') | ||
} | ||
testCompileClasspath 'org.jetbrains:annotations:21.0.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.
testCompileClasspath 'org.jetbrains:annotations:21.0.1' | |
testCompileOnly 'org.jetbrains:annotations:21.0.1' |
shouldn't be that with Only
suffix? As *Classpath configurations are intended to be resolvable, but not consumable
https://discuss.gradle.org/t/what-is-a-configuration-which-cant-be-directly-resolved/30721
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.
It was testCompileClasspath
before, so I suggest we address it separately, as this PR is something massive already :)
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.
okay, didn't notice it was moved
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 will try to polish this PR and reduce the changeset while fixing the merge conflict 👍
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.
Done
# Conflicts: # core/build.gradle
I just found a regression in the generated POM, will fix 👍 |
The issue is fixed now (and a check added that verifies that every property of POM dependencies in set (in that case, |
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 think this is looks tidier, so LGTM.
In general, it feels uncomfortable that we have to do so much scripting for all this - I wouldn't claim to understand much of what's going on here or why.
Is there some feedback we should be giving for the Shadow plugin so that they can fill any feature gaps, rather than us?
After #4171, we no longer need excludes, so I took a chance to make our build file shorter :)