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

Refactor Gradle build #1862

Merged
merged 38 commits into from
Nov 6, 2018
Merged

Refactor Gradle build #1862

merged 38 commits into from
Nov 6, 2018

Conversation

cbeams
Copy link
Contributor

@cbeams cbeams commented Nov 4, 2018

This PR supersedes @devinbileck's similar work at #1808. It makes a number of similar changes, does a few things differently, and then goes a good deal further toward simplifying the build arrangement. This is work that I've been meaning to do since reconsolidating into a single repository, but am just now getting to (thanks Devin for spurring that on).

There is now just a single build.gradle file in the root directory that uses configure blocks to configure each project in turn. It's important to understand that these blocks are additive. That is, it's possible to configure several projects at once in a single configure block, and then further configure each individual project in a dedicated configure block. You can see this being done in the way that all subprojects with a main method are configured with the application plugin in a single configure block, and then how each of those projects are further configured individually with their specific dependencies, etc, in subsequent blocks.

The bottom line is that we now have one-stop shopping for all our build needs, while still keeping concerns separated. Much cruft and duplication has been removed, and we're not doing anything really fancy with the build anywhere. It should be possible, with a minimum of ramp-up time for contributors not too familiar with Gradle to understand what's going on and to be able to make what changes they need. If you are one of those contributors, and find this new arrangement otherwise, please let me know.

cbeams added 30 commits November 4, 2018 09:16
This was vestige from when desktop was managed as a separate repository.
 - Move gradle-witness.jar to top level
 - Extract gradle-witness.gradle build file to top level

Moving these files out of the gradle/ dir and the main build file to the
top level of the repository calls them out and makes their presence and
function more obvious. It also removes a lot of noise from the main
build file.
This was a vestige from when desktop was managed as a separate
repository.
This is used only in common/build.gradle and is obsolete here.
It is no longer necessary to publish Maven metadata about common, core
and other submodules as they are no longer managed as separate libraries
in separate repositories. The only way these modules should be getting
referenced is from within applications in this repository such as
desktop, statsnode, etc. Essentially, we're no longer publishing our
libraries for public consumption.
Not all subprojects need it, but most do.
Previously, when assets was managed as a separate library and
repository, its Javadoc was published via Jitpack and linked to from the
documentation at https://bisq.network. This is no longer the case, and
therefore building the Javadoc jar is no longer necessary.
And also relocate gradle-witness jar and config (back) to root-level
gradle/witness dir.
This looks to have been a copy and paste error when originally setting
up the seednode build.
This was needed at the root level for a while when first porting the
build back to the monorepo, but is no longer necessary now.
@cbeams cbeams self-assigned this Nov 4, 2018
@cbeams cbeams mentioned this pull request Nov 4, 2018
@mrosseel
Copy link
Contributor

mrosseel commented Nov 4, 2018

hi Chris thx for the effort, could you have a quick look at the http-api gradle config to assess if it fits in the new structure?

https://github.com/mrosseel/bisq/blob/http-api/http-api/build.gradle

@cbeams
Copy link
Contributor Author

cbeams commented Nov 4, 2018 via email

@mrosseel
Copy link
Contributor

mrosseel commented Nov 4, 2018

Looks like it’ll be straightforward to integrate. Happy to help when needed.

great, once it's merged in master we'll have a go at it and call for help if necessary :)

Copy link
Member

@devinbileck devinbileck left a comment

Choose a reason for hiding this comment

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

utACK. Looks good to me. Thanks for putting this together!
Just have a few minor comments.

}

dependencies {
testCompile 'junit:junit:4.12'
Copy link
Member

Choose a reason for hiding this comment

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

Since you have defined the junit version above, I assume this can be changed to the following?
testCompile "junit:junit:$junitVersion"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I extracted that variable when there were still multiple declarations of the same junit dependency across the projects. Now, however, there is just one declaration applied to all subprojects in the configure(subprojects) block, so I'll simply remove the variable as it's no longer needed. The practice here is to extract these variables only as needed to eliminate duplication. If there's just one declaration, the version can remain inline.

build.gradle Outdated
compile('com.googlecode.json-simple:json-simple:1.1.1') {
exclude(module: 'junit')
}
compile 'org.springframework:spring-core:4.3.6.RELEASE'
Copy link
Member

Choose a reason for hiding this comment

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

Since you have the spring version defined above, I assume this can be changed to the following?
compile "org.springframework:spring-core:$springVersion"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, an oversight on my part, thanks. Fixed in 978fd2d.

apply plugin: 'witness'
apply from: '../gradle/witness/gradle-witness.gradle'

version = '0.8.0-SNAPSHOT'
Copy link
Member

Choose a reason for hiding this comment

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

A comment that is out of scope for this PR. But perhaps this should be taken out of the gradle file and put into a resource file, similar to how it is done for pricenode?
version = file("src/main/resources/version.txt").text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I cleaned up a number of unnecessary, unused, or just wrong instances of version metadata elsewhere in this PR, but in this case, the assignment of the $project.version variable affects the name of the resulting jar and shadow jar, so it's not quite as simple as extracting it to a resource file. There is also a lot of duplication of the current version throughout the codebase and supporting shell scripts that are used during releases. As you mention, it's a topic for another, more comprehensive PR that really cleans this stuff up.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK - works for me. Thanks for cleaning up the build scripts @cbeams and @devinbileck!

@cbeams cbeams merged commit 978fd2d into bisq-network:master Nov 6, 2018
cbeams added a commit that referenced this pull request Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants