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

Organize Misq with multi-project Gradle build system #1

Merged
merged 10 commits into from Jun 5, 2021

Conversation

ghubstan
Copy link
Member

@ghubstan ghubstan commented Jun 3, 2021

  • 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.

- 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.
@ghubstan ghubstan requested review from cbeams and a user June 3, 2021 16:09
Copy link
Contributor

@chimp1984 chimp1984 left a 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

jfx/build.gradle Outdated Show resolved Hide resolved
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

utACK

@ghubstan ghubstan self-assigned this Jun 3, 2021
Copy link
Member

@cbeams cbeams left a 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.

@cbeams
Copy link
Member

cbeams commented Jun 3, 2021

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:

Has anyone successfully built Bisq on an M1 Mac? I've got the Azul build of JDK 11 and that works fine, but am getting hung up on a protobuf dependency that doesn't exist for the architecture.

$ ./gradlew build
FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':proto:generateProto'.
> Could not resolve all files for configuration ':proto:protobufToolsLocator_protoc'.
   > Could not find protoc-3.10.0-osx-aarch_64.exe (com.google.protobuf:protoc:3.10.0).
     Searched in the following locations:
         https://repo.maven.apache.org/maven2/com/google/protobuf/protoc/3.10.0/protoc-3.10.0-osx-aarch_64.exe

Looks like there's a protobuf PR that fixes the problem by just using the osx/x86_64 dependency, but that PR hasn't been merged. To use this workaround on the Bisq side, it looks like it would be necessary to hack the proto:protobufToolsLocator_protoc gradle configuration to point to the osx/x86_64 protoc.exe url.

Just wondering if anyone has already done this, or if there's some other way folks have gotten Bisq building on M1.

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.

@ghubstan
Copy link
Member Author

ghubstan commented Jun 3, 2021

NACK. gradle-wrapper.jar isn't checked in

Added by 64d467b

Copy link
Member

@cbeams cbeams left a 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).

gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
@cbeams
Copy link
Member

cbeams commented Jun 4, 2021

@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.

commit 648d9a72edb43bc6b1715cf6db295b5263b7dcd1
Author: Chris Beams <chris@beams.io>
Date:   Fri Jun 4 10:41:34 2021 +0200

    Work around missing protobuf deps on M1 Mac
    
    As per https://github.com/grpc/grpc-java/issues/7690#issuecomment-747196215

diff --git buildSrc/gen-protos.gradle buildSrc/gen-protos.gradle
index 5724849..2b94f6f 100644
--- buildSrc/gen-protos.gradle
+++ buildSrc/gen-protos.gradle
@@ -11,8 +11,12 @@ sourceSets.main {
     java.srcDirs += [generatedProtoSrcDir]
 }
 
+// workaround for M1 Mac systems
+def arch = System.getProperty('os.arch') == 'aarch64' ? ':osx-x86_64' : ''
+
 protobuf {
-    protoc { artifact = 'com.google.protobuf:protoc:3.12.4' }
+    protoc { artifact = "com.google.protobuf:protoc:3.12.4$arch" }
     generateProtoTasks { all() }
     generatedFilesBaseDir = "$projectDir/build/generated/source"
 }
+
diff --git grpc/build.gradle grpc/build.gradle
index ee63860..406d4e3 100644
--- grpc/build.gradle
+++ grpc/build.gradle
@@ -39,13 +39,16 @@ sourceSets.main {
     java.srcDirs += [generatedProtoSrcDir, generatedGrpcProtoSrcDir]
 }
 
+// workaround for M1 Mac systems
+def arch = System.getProperty('os.arch') == 'aarch64' ? ':osx-x86_64' : ''
+
 protobuf {
     protoc {
-        artifact = 'com.google.protobuf:protoc:3.12.4'
+        artifact = "com.google.protobuf:protoc:3.12.4$arch"
     }
     plugins {
         grpc {
-            artifact = 'io.grpc:protoc-gen-grpc-java:1.38.0'
+            artifact = "io.grpc:protoc-gen-grpc-java:1.38.0$arch"
         }
     }
     generateProtoTasks {

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.

@cbeams
Copy link
Member

cbeams commented Jun 4, 2021

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. -mac), not by architecture (e.g. -x86_64, -aarch64), so I don't know what's missing just yet.

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
@ghubstan
Copy link
Member Author

ghubstan commented Jun 4, 2021

Why not model modules as Gradle subprojects? Why take the buildSrc route and apply from: as has been done here?

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 cross project configuration via the subprojects {} construct. After more research on subject, I am more convinced we should not use the Gradle subprojects construct. Reasons follow.

  • As I mentioned, Gradle devs recomend against use of the subprojects {} construct: "Another, discouraged, way to share build logic between subproject is cross project configuration via the subprojects {} and allprojects {} DSL constructs. With cross configuration, build logic can be injected into a subproject and this is not obvious when looking at the subproject’s build script, making it harder to understand the logic of a particular subproject."
    Reference: https://docs.gradle.org/current/userguide/sharing_build_logic_between_subprojects.html#sec:convention_plugins_vs_cross_configuration

  • Coupled projects cannot use the Gradle parallel build: "A very common way for projects to be coupled is by using configuration injection. It may not be immediately apparent, but using key Gradle features like the allprojects and subprojects keywords automatically cause your projects to be coupled." I never got Bisq subprojects to build in parallel, without being certain of the reason. I assumed it was because the subprojects were built out of order. As per the docs, that is the case: "The consequence of coupling during execution phase is that if gradle is invoked with the parallel option, one project task runs too late to influence a task of a project building in parallel. Gradle does not attempt to detect coupling and warn the user, as there are too many possibilities to introduce coupling." This trivial Misq build does not indicate the parallel build feature is certain to work as the code base grows, but it is enabled. After OS startup, my build is 5s. Subsequent builds are <1s. (The PR's protos have been stripped of all the POCs' proto msg defs, but <1s builds are the norm for https://github.com/ghubstan/misq-poc-ii on my 16 core machine. I'm not suggesting this same trivial build using the subprojects {} construct won't build in parallel just as quickly. I do assume the parallel build feature will break as the project grows.
    Ref: https://docs.gradle.org/current/userguide/multi_project_configuration_and_execution.html#sec:decoupled_projects

  • The incubating configuration on demand feature is recommended over configuration injection in Gradle 7, and it will not work for coupled subprojects relying on configuration injection. As per the Gradle doc: "The feature should work very well for multi-project builds that have decoupled projects." I think this is the case for this PR's build structure. If we use coupled subprojects {} with configuration injection, I am pretty sure we will not be able to use the configuration on demand feature (or parallel builds) at an early stage of development.
    Ref: https://docs.gradle.org/current/userguide/multi_project_configuration_and_execution.html#sec:configuration_on_demand

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 if(:proja) {} if(:projb) {} conditional logic is creeping in.

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:

configure([project(':cli'),
		       project(':daemon'),
		       project(':desktop'),
		       project(':monitor'),
		       project(':relay'),
		       project(':seednode'),
		       project(':statsnode'),
		       project(':pricenode'),
		       project(':inventory'),
		       project(':apitest')]) {

		apply plugin: 'application'
		....
	}    

This PR has two applications, with id 'application' present in two project build files. This plugin's apply and the rest of the modules' build configs are fully visible in the editor.

Copy link
Contributor

@chimp1984 chimp1984 left a 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.

@ghubstan
Copy link
Member Author

ghubstan commented Jun 4, 2021

Do we change the license handling style to not include it in all files anymore but only use one global?

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.
@cbeams
Copy link
Member

cbeams commented Jun 5, 2021

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.

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:

@cbeams
Copy link
Member

cbeams commented Jun 5, 2021

I do not want to work against tool devs' recommendations, especially in relation to something as important as overall build organization.

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!

@cbeams
Copy link
Member

cbeams commented Jun 5, 2021

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.

@ghubstan ghubstan requested a review from a user June 5, 2021 11:57
@ghubstan
Copy link
Member Author

ghubstan commented Jun 5, 2021

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.
I hope next release has recently merged fix.

@ghubstan ghubstan merged commit 0e17de9 into bisq-network:main Jun 5, 2021
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.

None yet

3 participants