feat: Packaged Executable #1125

bdferris-v2 · 2022-04-27T16:48:18Z

Summary:

Introduces functionality to package the GTFS Validator as an installable application, including a bundled JRE, to make it easier for users to run the validator. As discussed in issue #1112, https://bit.ly/gtfs-validator-packaged-exe, and #1124.

Note: This is an initial draft review to get agreement on package structure before additional follow-up work.

Expected behavior:

Prerequisites:

A Windows or Mac OS build machine (I haven't tested on Linux, but it should theoretically work there as well)
A Java SDK that includes jlink and jpackage
If building on Windows, have https://wixtoolset.org/ installed on your path for Windows Installer support

To package:

run ./gradlew jpackage

Expected:

You should find a "GTFS Validator" application and installer in the app/pkg/build/jpackage directory.

The current application is just a simple wrapper around the existing CLI. On Windows, you can run the validator by dragging a GTFS zip file onto the application shortcut. On Mac OS, you still have to run it from the command-line while we wait on some simple GUI support (e.g. GTFS\ Validator.app/Contents/MacOS/GTFS\ Validator path/to/your/gtfs.zip).

Future work will include:

refactor the main sub-project to separate table+validation code from the cli code
adding a simple UI for selecting a GTFS feed, output directory, configuring the validator, etc

…uava ClassPath. Guava's ClassPath scanning has issues when run against Java Modules and its own source code advises you to use ClassGraph instead: https://github.com/google/guava/blob/master/guava/src/com/google/common/reflect/ClassPath.java This change will better support running the validator as a Java Module in a packaged runtime. modified: core/build.gradle modified: core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGenerator.java modified: core/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsFeedLoader.java modified: core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java

…tive application on Windows and Mac OS. Right now, it's just a simple wrapper around the CLI app.

CLAassistant · 2022-04-27T16:48:24Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

isabelle-dr · 2022-04-28T19:39:47Z

@bdferris FYI, we recently implemented the acceptance tests that runs this validator on all the datasets we have on the Mobility Database and flags if we had previously valid datasets that become invalid with the PR.
It is part of our PR process to run these tests before merging any PR, but they currently fail if a PR is opened from a forked repo (see Issue #1094).
We haven't had a chance to have a look into it yet, but this is something that we will need to solve so we can run them on this PR.

cc @asvechnikov2

barbeau · 2022-04-28T19:42:45Z

I believe I have a workaround for #1094 that I'll comment on shortly.

barbeau · 2022-04-28T21:21:55Z

After #1102 is merged and then the main branch is merged into this PR, then the acceptance tests should pass.

bdferris-v2 · 2022-04-28T21:32:22Z

I think there might be some other test failures unrelated to issue #1094, as discussed in issue #1129? Curious if you all have thoughts on that.

…nsole, for easier debugging when running the app in non command-line mode.

barbeau

Thanks @bdferris-v2, this looks good!

Some thoughts in-line.

app/pkg/build.gradle

app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/Main.java

barbeau · 2022-05-03T19:13:42Z

Also, looks like the Javadoc generation GitHub Action is failing:
https://github.com/MobilityData/gtfs-validator/runs/6276946492?check_suite_focus=true

....
  (package javax.annotation is declared in the unnamed module, but module org.mobilitydata.gtfsvalidator.app.pkg does not read it)
/home/runner/work/gtfs-validator/gtfs-validator/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/RuntimeExceptionInLoaderError.java:3: error: package com.google.common.base is not visible
import com.google.common.base.Strings;
                        ^
  (package com.google.common.base is declared in module com.google.common, but module org.mobilitydata.gtfsvalidator.app.pkg does not read it)
100 errors

> Task :aggregateJavadocs FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':aggregateJavadocs'.
> Javadoc generation failed. Generated Javadoc options file (useful for troubleshooting): '/home/runner/work/gtfs-validator/gtfs-validator/build/tmp/aggregateJavadocs/javadoc.options'

bdferris-v2 · 2022-05-03T20:30:06Z

@barbeau the aggregate javadoc issue is actually slightly tricky. Javadoc generation appears to get tripped up for the :app:pkg sub-project because of the previously discussed tricks I used to get Java Modules working (shadow jar wrapped as a single module). Specifically, Javadoc appears to be adding the the shadow jar its classpath along with all the other project dependencies, which is causing a bunch of duplicate class warnings.

There seem to be two options for fixing this:

We try to fully modularize the entire project. I'm wary of going down this road for now.
We try to exclude the :app:pkg project from Javadoc generation. It only has a single dummy class anyway, so we're not missing much there.

The trick with option # 2 is that we are using the nebula-aggregate-javadocs plugin from Netflix for project-wide javadoc aggregation. This plugin doesn't appear to have a way of excluding a project (I've looked at the source). This plugin also hasn't been updated in five years?

As an alternative, there are newer Gradle plugins that support javadoc aggregation. Specifically, I just tried the io.freefair.aggregate-javadoc-jar, which is currently being actively maintained. Per the source, it's possible to exclude particular projects with a "javadoc { enabled = false }" clause in a sub-project. I've verified this behavior on own project. It seems to generate aggregated javadoc in build/docs/javadoc/ like the old plugin, though the target name has changed:

aggregateJavadocs => aggregateJavadoc (the s is dropped)

So I'd need to update the workflow as well.

I wanted to get a thumbs up from someone before I swapped out the plugin, in case anyone has any concerns.

barbeau · 2022-05-03T21:07:58Z

I noticed the Netflix plugin hadn't been updated in a while too, so I'm 👍 for changing to io.freefair.aggregate-javadoc-jar.

Specifically, switch the project to use the io.freefair.aggregate-javadoc-jar plugin and disable Javadoc generation for the :app:pkg sub-project. Javadoc generation appears to get tripped up for the :app:pkg sub-project because of the previously discussed tricks I used to get Java Modules working (shadow jar wrapped as a single module). Specifically, Javadoc appears to be adding the the shadow jar its classpath along with all the other project dependencies, which is causing a bunch of duplicate class warnings. There seem to be two options for fixing this: 1) We try to fully modularize the entire project. I'm wary of going down this road for now. 2) We try to exclude the :app:pkg project from Javadoc generation. It only has a single dummy class anyway, so we're not missing much there. The trick with option # 2 is that we are using the nebula-aggregate-javadocs plugin from Netflix for project-wide javadoc aggregation. This plugin doesn't appear to have a way of excluding a project (I've looked at the source). This plugin also hasn't been updated in five years? As an alternative, there are newer Gradle plugins that support javadoc aggregation. Specifically, I just tried the io.freefair.aggregate-javadoc-jar, which is currently being actively maintained. Per the source, it's possible to exclude particular projects with a "javadoc { enabled = false }" clause in a sub-project. I've verified this behavior on own project. It seems to generate aggregated javadoc in build/docs/javadoc/ like the old plugin, though the target name has changed: aggregateJavadocs => aggregateJavadoc (the s is dropped)

maximearmstrong · 2022-05-03T23:40:15Z

I tested ./gradlew aggregateJavadoc and it works well with io.freefair.aggregate-javadoc-jar. Thank you for this @bdferris-v2 !
I was also able to run ./gradlew jpackage successfully, but I'm on MacOs so I can't test the application behavior for now. I'll be back with my full code review.

I took the liberty of replacing the existing static architecture diagram with a Mermaid diagram that can be edited directly in Markdown.

maximearmstrong · 2022-05-04T20:47:33Z

After #1102 is merged and then the main branch is merged into this PR, then the acceptance tests should pass.

#1102 is now merged, the acceptance tests should pass when the next commit is pushed.

maximearmstrong

Thanks @bdferris-v2, this is great :)

I think the module naming is ok. After reading the documentation I think it's obvious, although I'm not sure it would be obvious to someone coming to the project fresh without reading the docs. But I can't think of better naming myself, so I'd say let's go with it :).

After reviewing everything, I agree with this. I think the architecture you provided is the best possible for now. I don't have anything else to add that Sean hasn't already mentioned.

bdferris-v2 · 2022-05-05T20:16:56Z

Ok, at this point, I think this PR is ready for full review (hopefully we've already done most of the work). Acknowledged that I've punted a number of implementation details into follow-up work tracked in #1134 and #1135. However, I think the PR is in a usable state and worth committing before we move onto subsequent iterations.

barbeau · 2022-05-05T20:57:30Z

@bdferris-v2 Could you add the prerequisites and:

./gradlew jpackage

...that are in the first PR comment in BUILD.md as well?

The prerequisites in the PR comment say "A Java SDK that includes jlink and jpackage". Can you expand on how one would get ahold of such an SDK?

bdferris-v2 · 2022-05-05T23:01:44Z

@barbeau done, per 830a9d3

barbeau

This is looking good @bdferris-v2! A few minor comments.

barbeau · 2022-05-05T20:26:50Z

docs/ARCHITECTURE.md

-![architecture schema](https://user-images.githubusercontent.com/35747326/101182386-610e9400-3624-11eb-84b9-ec935e44aa2b.png)
+`gtfs-validator` is composed from a number modules, as shown in the following dependency diagram:
+
+```mermaid


😍 Didn't know you could do this in Markdown! Very cool!

app/pkg/build.gradle

docs/BUILD.md

bdferris-v2 · 2022-05-10T21:01:34Z

@barbeau @maximearmstrong I think we might be getting close here. Any additional concerns?

barbeau

👍

maximearmstrong

I agree, we are very close. One thing, I think we need a section in the README.md on how to use the installable application. I also added a comment related to that in my review.

docs/BUILD.md

barbeau · 2022-05-10T22:15:16Z

One thing, I think we need a section in the README.md on how to use the installable application. I also added a comment related to that in my review.

I almost commented saying something similar, but after more thought I would wait on this, because it's going to change significantly when the UI is added in an upcoming PR. I think the existing note saying that it's under active development in the build section is probably enough for now as it's not ready to be user-facing yet.

maximearmstrong · 2022-05-10T22:16:39Z

One thing, I think we need a section in the README.md on how to use the installable application. I also added a comment related to that in my review.

I almost commented saying something similar, but after more thought I would wait on this, because it's going to change significantly when the UI is added in an upcoming PR. I think the existing note saying that it's under active development in the build section is probably enough for now as it's not ready to be user-facing yet.

I agree. I was about to say the same now that the note have been added.

maximearmstrong · 2022-05-10T22:18:56Z

@bdferris-v2 I approved this PR. Thank you for this contribution! 🚀

maximearmstrong · 2022-05-10T22:21:12Z

@bdferris-v2 It seems that we are only missing the signature of the license/cla to be able to merge.

bdferris-v2 · 2022-05-10T22:25:05Z

@maximearmstrong I did sign the CLA, like two weeks ago :)

When I go to https://cla-assistant.io/MobilityData/gtfs-validator, I see the following:

Is there something else I need to do? Anything you want to check on your end?

barbeau · 2022-05-10T22:39:23Z

@maximearmstrong I think my permissions on the MobilityData org changed at some point and I can no longer see the list of signed CLAs for MobilityData:

I also can't see the current hooks/3rd party tools integrations on this repo settings either. IIRC I'm the one that set this up originally.

Could you try going to https://cla-assistant.io/ and see if you can see @bdferris-v2's CLA there?

I'm wondering if something is wrong with the CLA bot, although it looks like a 3rd party contributor was recognized as having signed the CLA by the bot in Feb:
#1090

I'm not sure if something re: permissions would have changed since then that could have broken it. Maybe related to MobilityData/gtfs-realtime-validator#150?

maximearmstrong · 2022-05-10T22:40:45Z

@bdferris-v2 Oh, it seems the issue is that your other account @bdferris is also a committer on the project. Since you got locked out of that account and signed the CLA with your new account, I think we can merge without any problem.

bdferris-v2 · 2022-05-10T22:47:06Z

Yeah, I'm sure that's going to cause continued confusion. The original account is still associated with my primary email, so it tends to get picked up by default despite my best efforts.

Either way, I'm going to go ahead and merge. Thanks!

maximearmstrong · 2022-05-10T22:59:30Z

Yeah, I'm sure that's going to cause continued confusion. The original account is still associated with my primary email, so it tends to get picked up by default despite my best efforts.

No worries. I have reconfigured the CLA assistant using my account, so we'll see. You may need to sign it again on your next PR. Thanks again for your contribution!

bdferris added 3 commits April 25, 2022 22:41

Initial entry for a minimal packaged validator app, to be run as a na…

bc97f95

…tive application on Windows and Mac OS. Right now, it's just a simple wrapper around the CLI app.

Add some documentation comments to app.gui.Main

5c32c34

isabelle-dr mentioned this pull request Apr 27, 2022

3.1.0 Release plan - Validator as Packaged Executable + UI for results #1124

Closed

isabelle-dr added this to the v3.1.0 - Result UI + Validator as Packaged Executable milestone Apr 28, 2022

barbeau mentioned this pull request Apr 28, 2022

ci: Remove upload of validation reports to Google Cloud #1102

Merged

3 tasks

bdferris-v2 mentioned this pull request Apr 28, 2022

End to End workflow failure of PR #1129

Closed

Write flogger statments to ~/gtfs-validator.log in addition to the co…

a929293

…nsole, for easier debugging when running the app in non command-line mode.

barbeau reviewed May 3, 2022

View reviewed changes

Update copyright headers on new files.

8f26673

Add details about app:gui and app:pkg sub-projects to ARCHITECTURE.md.

398ed25

I took the liberty of replacing the existing static architecture diagram with a Mermaid diagram that can be edited directly in Markdown.

This was referenced May 4, 2022

Add a simple GUI to the desktop validation app in :app:gui #1134

Closed

Refactor desktop validation app to call validation code directly instead of cli.Main #1135

Closed

Add and update TODOs to reference github issues.

2251f94

maximearmstrong reviewed May 4, 2022

View reviewed changes

bdferris added 2 commits May 5, 2022 13:08

Update app to use a default output directory of ~/GtfsValidator.

a2f0567

Merge branch 'master' into packaged_exe

37835df

Add build instructions for the installable application in BUILD.md.

830a9d3

bdferris-v2 marked this pull request as ready for review May 5, 2022 23:01

barbeau reviewed May 10, 2022

View reviewed changes

bdferris added 3 commits May 10, 2022 09:36

Merge branch 'master' into packaged_exe

1b9eb50

Remove $ from command line in BUILD.md.

dc11267

Note requirement for Wix when building package on Windows.

c358835

bdferris-v2 mentioned this pull request May 10, 2022

Refactor handling of project version numbers and tags #1139

Closed

barbeau approved these changes May 10, 2022

View reviewed changes

maximearmstrong suggested changes May 10, 2022

View reviewed changes

docs/BUILD.md Show resolved Hide resolved

Add note about how app is under active development.

5e02c89

maximearmstrong approved these changes May 10, 2022

View reviewed changes

bdferris-v2 merged commit be7c4a4 into MobilityData:master May 10, 2022

bdferris-v2 mentioned this pull request May 3, 2023

Gradle aggregrateJavadocJar hangs indefinitely. #1431

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Packaged Executable #1125

feat: Packaged Executable #1125

bdferris-v2 commented Apr 27, 2022

CLAassistant commented Apr 27, 2022

isabelle-dr commented Apr 28, 2022

barbeau commented Apr 28, 2022

barbeau commented Apr 28, 2022 •

edited

Loading

bdferris-v2 commented Apr 28, 2022

barbeau left a comment

barbeau commented May 3, 2022

bdferris-v2 commented May 3, 2022

barbeau commented May 3, 2022

maximearmstrong commented May 3, 2022

maximearmstrong commented May 4, 2022

maximearmstrong left a comment

bdferris-v2 commented May 5, 2022

barbeau commented May 5, 2022 •

edited

Loading

bdferris-v2 commented May 5, 2022

barbeau left a comment

barbeau May 5, 2022

bdferris-v2 commented May 10, 2022

barbeau left a comment

maximearmstrong left a comment

barbeau commented May 10, 2022

maximearmstrong commented May 10, 2022

maximearmstrong commented May 10, 2022

maximearmstrong commented May 10, 2022

bdferris-v2 commented May 10, 2022

barbeau commented May 10, 2022

maximearmstrong commented May 10, 2022

bdferris-v2 commented May 10, 2022

maximearmstrong commented May 10, 2022

feat: Packaged Executable #1125

feat: Packaged Executable #1125

Conversation

bdferris-v2 commented Apr 27, 2022

CLAassistant commented Apr 27, 2022

isabelle-dr commented Apr 28, 2022

barbeau commented Apr 28, 2022

barbeau commented Apr 28, 2022 • edited Loading

bdferris-v2 commented Apr 28, 2022

barbeau left a comment

Choose a reason for hiding this comment

barbeau commented May 3, 2022

bdferris-v2 commented May 3, 2022

barbeau commented May 3, 2022

maximearmstrong commented May 3, 2022

maximearmstrong commented May 4, 2022

maximearmstrong left a comment

Choose a reason for hiding this comment

bdferris-v2 commented May 5, 2022

barbeau commented May 5, 2022 • edited Loading

bdferris-v2 commented May 5, 2022

barbeau left a comment

Choose a reason for hiding this comment

barbeau May 5, 2022

Choose a reason for hiding this comment

bdferris-v2 commented May 10, 2022

barbeau left a comment

Choose a reason for hiding this comment

maximearmstrong left a comment

Choose a reason for hiding this comment

barbeau commented May 10, 2022

maximearmstrong commented May 10, 2022

maximearmstrong commented May 10, 2022

maximearmstrong commented May 10, 2022

bdferris-v2 commented May 10, 2022

barbeau commented May 10, 2022

maximearmstrong commented May 10, 2022

bdferris-v2 commented May 10, 2022

maximearmstrong commented May 10, 2022

barbeau commented Apr 28, 2022 •

edited

Loading

barbeau commented May 5, 2022 •

edited

Loading