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

Tidy up core's build file #4207

Merged
merged 11 commits into from Sep 23, 2021
Merged

Tidy up core's build file #4207

merged 11 commits into from Sep 23, 2021

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Jun 18, 2021

After #4171, we no longer need excludes, so I took a chance to make our build file shorter :)

@bsideup bsideup added this to the next milestone Jun 30, 2021
@bsideup bsideup marked this pull request as ready for review June 30, 2021 09:28
@bsideup bsideup removed this from the next milestone Jul 16, 2021
shaded 'org.zeroturnaround:zt-exec:1.12', {
exclude(group: 'org.slf4j')
}
testCompileClasspath 'org.jetbrains:annotations:21.0.1'
Copy link
Contributor

@lanwen lanwen Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Member Author

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 :)

Copy link
Contributor

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

Copy link
Member Author

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 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@bsideup bsideup requested a review from kiview September 20, 2021 13:42
gradle/publishing.gradle Outdated Show resolved Hide resolved
@bsideup bsideup marked this pull request as draft September 21, 2021 11:32
@bsideup
Copy link
Member Author

bsideup commented Sep 21, 2021

I just found a regression in the generated POM, will fix 👍

@bsideup
Copy link
Member Author

bsideup commented Sep 21, 2021

The issue is fixed now (and a check added that verifies that every property of POM dependencies in set (in that case, version was missing due to the usage of platform(...), and changing it to resolvedConfiguration fixed it)

@bsideup bsideup marked this pull request as ready for review September 21, 2021 11:52
Copy link
Member

@rnorth rnorth left a 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?

@bsideup
Copy link
Member Author

bsideup commented Sep 23, 2021

@rnorth that was I was planning to do! I do feel like a good amount of this code could be moved to the plugin, and it will be beneficial to many Shadow users 👍

Have submitted #4478 for this

@bsideup bsideup merged commit d512ca5 into master Sep 23, 2021
@delete-merged-branch delete-merged-branch bot deleted the cleanup_build_file branch September 23, 2021 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants