-
Notifications
You must be signed in to change notification settings - Fork 62
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
Organize Misq with multi-project Gradle build system #1
Conversation
- Gradle version 7.0.2 and OpenJDK 16, are required. - The JDK's Java Modules (JPMS) framework is not used, except where necessary (JavaFX apps require jvm options specify modules). - Gradle projects are distinct (not sub-projects), but share common platforms (version constraints), dependencies, and build tasks. - Gradle task customization is defined in buildSrc/build.gradle, but can be overriden in module build files. - Gradle parallel build feature is used to shorten build times. This did not work in Bisq, but it is worth trying in latest Gradle version. - Common dependency declarations are defined in buildSrc/*.gradle files. - Common protobuf generation task is defined in buildSrc/gen-protos.gradle - The Gradle Platforms api is used to define dependency version constraints in a central location. Projects (modules) reference a platform, then declare specific dependencies without needing to specify the version. Referencing a platform does not declare any dependencies, it only constrains the dependency version when project declares it. - Gradle signature verification support for dependenices is not enabled. Enabling it requires creating gradle/verification-metadata.xml and manually configuring each failed verfication. See https://docs.gradle.org/7.0.2/userguide/dependency_verification.html Note: the gradle-witness plugin is not compatible with Gradle 7.0.2. Modules (Synonymous with Gradle Projects, not JPMS Modules) - Stub out module 'api' with common logging/test deps. - Stub out module 'application' with common logging/test deps. - Stub out module 'common', defining shared logging/test deps. - Stub out modules 'grpc' & 'cli' with grpc, protobuf, & common logging/test deps. - Stub out module 'jfx' with java-fx, and common logging/test deps. Jpackage v16 has been shown to work for simple JFX 16 app via cmd line. May use a modified Bisq:desktop:package.gradle script to invoke from the 'jfx:build.gradle', or see if a convenience plugin can work with jpackage 16. See https://github.com/petr-panteleyev/jpackage-gradle-plugin. - Stub out module 'p2p' with P2PService class, protobuf, & common logging/test deps. - Stub out module 'web' with common logging/test deps. Webapp framework choice is still being considered.
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.
Beside the mentioned class rename request utACk from my side, but it would be good if others like @cbeams who are experts on gradle add their review
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.
utACK
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.
NACK. gradle-wrapper.jar
isn't checked in, should be here:
$ tree gradle/wrapper/
gradle/wrapper/
└── gradle-wrapper.properties
0 directories, 1 file
results in:
air:~/lab/ghubstan-misq[main]
$ ./gradlew build
Error: Could not find or load main class org.gradle.wrapper.GradleWrapperMain
Caused by: java.lang.ClassNotFoundException: org.gradle.wrapper.GradleWrapperMain
will add any other substantive comments in another review, just wanted to get this off quickly.
Not a showstopper for this PR, but I just ran into the same issue trying to build Misq on my M1 Mac that I ran into trying to build Bisq on it the other day. This is from what I reported at keybase://chat/bisq#dev/5122:
No one else had said they got it running. The Protobuf PR I mentioned above that fixes this is protocolbuffers/protobuf#8557, and it actually just got merged 23 hours ago at time of writing. According to protocolbuffers/protobuf#8557 (comment), the latest v3.17.2 release doesn't contain the fix, but presumably the next release will. |
Added by 64d467b |
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.
Unfortunately, because of the above-mentioned issue with my M1 Mac, I haven't actually been able to take the build for a spin. So what follows are my observations just looking through the changes visually.
Why not model modules as Gradle subprojects? Why take the buildSrc
route and apply from:
as has been done here?
Gradle's approach to 'configuration injection' is an elegant enough way to handle applying the same dependencies (or any other build property) to multiple subprojects, and it's the way we do it now in Bisq.
For example, see https://github.com/bisq-network/bisq/blob/master/build.gradle#L97-L108, where the application
plugin is applied to 10 subprojects at the same time. The same can be done for common logging, protobuf, etc dependencies { }
blocks.
Any reason not to follow suit?
More generally, I've always found myself avoiding buildSrc
over the years, even after having thought I found good use cases for it. Perhaps this situation will be different, but I would lean toward omitting it if it's not strictly necessary.
It was also a pretty carefully considered decision to colocate all build configuration in a single, root build.gradle
file in the main Bisq codebase. In the end, most people that interact with the project don't know a lot about Gradle, and it's better to have one stop shopping
in a single file. I've found this true even for the experts. It cuts down on duplication, jumping around between directories, etc, and allows for maximizing the utility of the aforementioned 'configuration injection' technique. I think this single root file approach has held up pretty well now for quite a while in Bisq proper, and I'd recommend staying with it here.
Nit: Some files were copied over in the grpc
and p2p
modules that still have license headers from Bisq. Should probably strip those out.
Not strictly related to this PR, but I wanted to mention it here before I forget: The LICENSE file that @ripcurlx added in his initial commit (that this PR is based on) is GPLv3. Should be AGPLv3 (as Bisq itself is).
@ghubstan, I've worked around the problem on the M1 mac that I described above at #1 (comment). To avoid making a pull request against a pull request, I'll just paste the fix as a patch below. Could you please apply it? Thanks.
Note that the duplication seen above could easily have been avoided with proper Gradle subprojects in place and/or a single root-level build file. |
I'm now running into a "No toolkit found" error from JavaFX as described at https://stackoverflow.com/questions/53994490/getting-java-lang-runtimeexception-no-toolkit-found-error-on-running-javafx-a I assume this, too, has to do with the M1 aarch64 architecture not being supported by the published toolkits, but looking at https://repo.maven.apache.org/maven2/org/openjfx/javafx-graphics/16/, they're only generally qualified by os (e.g. |
Contains extra osxArch property to work around Mac M1 protobuf support bug, which may be fixed by google in next release.
The generated jfx distribution requires a wrapper around JfxMain. Main must be the dist's entry point. Renaming JfxMain as Main does not fix the dist runtime problem. See javafxports/openjdk-jfx#236
I do not want to work against tool devs' recommendations, especially in relation to something as important as overall build organization. From Gradle 7 docs I found out current Gradle devs -- assuming the docs' authors represent devs' opinions -- suggest not sharing build logic between subprojects using
To me, those are the most important reasons to not use subprojects {}. As for having a one stop shopping in a single build file, it gets subjective. I like having almost the complete build file visible in my editor, two clicks away: click project, open gradle.build. I think the Bisq build file is too long, and Another subjective preference is applying a plugin in a project's build file, instead of in the single build file, e.g., Bisq's build.gradle:
This PR has two applications, with |
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.
Do we change the license handling style to not include it in all files anymore but only use one global? Or was it removed because of the reference to Bisq (and not Misq). I think as we likely will run the project under Bisq brand/umbrella the Bisq reference can be kept. Misq is not much of an organisation yet.
I deleted license headers on @cbeams' request, because it refers to Bisq, not Misq. I'm not sure of the best/required license handling style. |
The 'org.openjfx.javafxplugin' logs a potentially misleading message if a JPMS module-info.java is missing from any project is it applied to (in this case, the jfx gradle project). This change adds a module-info.java, and build msg "Project :jfx => no module-info.java found" becomes "Project :jfx => 'network.misq.jfx' Java module". Many 3rd party libraries conform to the JPMS "Automatic Module" and "Unnamed Module" type structures, but not all, like the gprc libs. Should the misq:jfx project module ever require any 3rd party lib that does not conform to any JPMS module type, this change will have to be reverted, and misq builds will log "no module-info.java found" messages.
Indeed, I took it as an oversight that the license headers were included with the name Bisq. But agreed, it should rather stay Bisq as the umbrella project / organization and not be Misq in the license text. I had considered suggesting doing away with the per-license file headers, but I just researched it, and the (A)GPLv3 does specifically recommend adding the license reference in each source file. So let's just keep going with that. References: |
Thanks, @ghubstan. I wasn't aware of these recommendations, but on reading them, I do recall many of these conversations around cross-configuration preventing full parallelization of builds. Really appreciate the detailed write-up! |
I've approved the PR from my side, as my concerns have been addressed. I've still been unable to build successfully on my M1 Mac, but that's no reason to hold this up for others whom it does work for. |
|
Gradle version 7.0.2 and OpenJDK 16, are required.
The JDK's Java Modules (JPMS) framework is not used, except where
necessary (JavaFX apps require jvm options specify modules).
Gradle projects are distinct (not sub-projects), but share common
platforms (version constraints), dependencies, and build tasks.
Gradle task customization is defined in buildSrc/build.gradle, but
can be overriden in module build files.
Gradle parallel build feature is used to shorten build times.
This did not work in Bisq, but it is worth trying in latest
Gradle version.
Common dependency declarations are defined in buildSrc/*.gradle files.
Common protobuf generation task is defined in buildSrc/gen-protos.gradle
The Gradle Platforms api is used to define dependency version
constraints in a central location. Projects (modules) reference a platform,
then declare specific dependencies without needing to specify the
version. Referencing a platform does not declare any dependencies,
it only constrains the dependency version when project declares it.
Gradle signature verification support for dependenices is not enabled.
Enabling it requires creating gradle/verification-metadata.xml and
manually configuring each failed verfication.
See https://docs.gradle.org/7.0.2/userguide/dependency_verification.html
Note: the gradle-witness plugin is not compatible with Gradle 7.0.2.
Modules (Synonymous with Gradle Projects, not JPMS Modules)
Stub out module 'api' with common logging/test deps.
Stub out module 'application' with common logging/test deps.
Stub out module 'common', defining shared logging/test deps.
Stub out modules 'grpc' & 'cli' with grpc, protobuf, & common logging/test deps.
Stub out module 'jfx' with java-fx, and common logging/test deps.
Jpackage v16 has been shown to work for simple JFX 16 app via cmd line.
May use a modified Bisq:desktop:package.gradle script to invoke from
the 'jfx:build.gradle', or see if a convenience plugin can work with
jpackage 16. See https://github.com/petr-panteleyev/jpackage-gradle-plugin.
Stub out module 'p2p' with P2PService class, protobuf, & common logging/test deps.
Stub out module 'web' with common logging/test deps.
Webapp framework choice is still being considered.