Upgrade to Java 17 and gradle 7.4.2. #188

cmnbroad · 2022-10-11T14:13:30Z

Moves the doclets (help tab completion and WDL generation) over to the new doclet API introduced in Java 11. Requires a major version upgrade.

codecov · 2022-10-11T15:52:58Z

Codecov Report

Base: 76.69% // Head: 77.29% // Increases project coverage by +0.59% 🎉

Coverage data is based on head (86ba9c1) compared to base (bc47857).
Patch coverage: 76.80% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #188      +/-   ##
============================================
+ Coverage     76.69%   77.29%   +0.59%     
+ Complexity      762      742      -20     
============================================
  Files            33       41       +8     
  Lines          2609     2757     +148     
  Branches        505      472      -33     
============================================
+ Hits           2001     2131     +130     
- Misses          409      448      +39     
+ Partials        199      178      -21

Impacted Files	Coverage Δ
...adinstitute/barclay/help/BarclayDocletOptions.java	`0.00% <0.00%> (ø)`
...ava/org/broadinstitute/barclay/help/WDLDoclet.java	`65.38% <0.00%> (+2.42%)`	⬆️
...ava/org/broadinstitute/barclay/utils/JVMUtils.java	`0.00% <ø> (-27.28%)`	⬇️
...a/org/broadinstitute/barclay/help/DocWorkUnit.java	`54.94% <10.52%> (-30.77%)`	⬇️
...oadinstitute/barclay/help/BarclayDocletOption.java	`64.28% <64.28%> (ø)`
...a/org/broadinstitute/barclay/help/DocletUtils.java	`68.18% <70.00%> (+14.84%)`	⬆️
...titute/barclay/help/DefaultDocWorkUnitHandler.java	`76.83% <75.00%> (+3.16%)`	⬆️
...nstitute/barclay/help/scanners/CommentScanner.java	`83.33% <83.33%> (ø)`
...rclay/help/scanners/JavaLanguageModelScanners.java	`86.36% <86.36%> (ø)`
...va/org/broadinstitute/barclay/help/HelpDoclet.java	`85.63% <86.76%> (+6.66%)`	⬆️
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lbergelson

@cmnbroad A few comments. There's some stuff that could be cleaned up using the new switch expressions and instanceof pattern matching but it's not necessary. I have to say I've never 100% understood how this all works so I'm not sure if I'm catching wierd errors in scanner design or anything like that. They seem to be consistent with each other though.

I noticed that this uses a Reporter in a lot of places instead of a the logger. Is it possible to remove the logger dependency and use a reporter in all places here?

Are the com.sun classes always available now through the toolprovider methods? Are there any gotchas about those like there used to be?

lbergelson · 2022-10-17T21:52:29Z

build.gradle

+            name = isRelease ? "SonaType" : "Artifactory"
+            url =  isRelease ? "https://oss.sonatype.org/service/local/staging/deploy/maven2/" : "https://broadinstitute.jfrog.io/broadinstitute/libs-snapshot-local/"
+            credentials {
+                username = isRelease ? project.findProperty("sonatypeUsername") : System.env.ARTIFACTORY_USERNAME


It's very silly that one of these is set through the environment one is a gradle property. Maybe we should unify them. It's entirely a relic based on my first draft at publishing these things I believe.

lbergelson · 2022-10-17T21:55:44Z

build.gradle

@@ -119,61 +116,67 @@ artifacts {
 * Sign non-snapshot releases with our secret key.  This should never need to be invoked directly.
 */
 signing {
-    required { isRelease && gradle.taskGraph.hasTask("uploadArchives") }
+    required { isRelease && gradle.taskGraph.hasTask("publish") }
    sign configurations.archives


I think you might need to change this to sign publishing.publications at least based on how gatk and htsjdk were updated

Ok, will try that.

lbergelson · 2022-10-17T21:57:24Z

src/main/java/org/broadinstitute/barclay/help/BarclayDocletOption.java

+ */
+abstract public class BarclayDocletOption implements Doclet.Option {
+    // record to keep all of the components required for an option
+    private record DocletOptionRecord(


neat, a record

A bit of a strange use, too bad you can't have a record with an abstract method.

You could make it just a record that contains a lambda that does the processing I think if you wanted to make the class just directly a record instead of this indirection. You couldn't make helper subclasses then though.

I left this as is, and added the helper subclasses.

lbergelson · 2022-10-18T18:54:19Z

src/main/java/org/broadinstitute/barclay/help/BashTabCompletionDoclet.java

     * This can be thought of as a set of such argument relationships and does not have any ordering scheme.
-     *
+     * <p>


I would love to change to markdown instead of html in the templates.

lbergelson · 2022-10-18T18:56:19Z

src/main/java/org/broadinstitute/barclay/help/BashTabCompletionDoclet.java

+                    Option.Kind.STANDARD,
+                    "<string>") {
+                @Override
+                public boolean process(String option, List<String> arguments) {


So much boiler plate. It looks like most (maybe all?) follow the same exact initialization. I think you could use something like this and remove all the redundant initialization and just provide the necessary function override.

private static abstract class SingleStandardOption extends BarclayDocletOption { public SingleStandardOption(String name){ super(List.of(name), name, 1, Kind.STANDARD, "<string>"); } }

Technically, that would be SingleStandardStringWithOneArgumentAndNoAliasesOption. But yeah, that would eliminate least some boilerplate.

Done - I added this as a static class inside BarclayDocletOption so it can also be used in the other doclets.

lbergelson · 2022-10-18T19:27:34Z

src/main/java/org/broadinstitute/barclay/help/HelpDoclet.java

-        // Get a list of all the features and groups that we'll actually retain
-        workUnits = computeWorkUnits();
+        // scan all included elements for DocumentedFeatures
+        workUnits = JavaLanguageModelScanners.getWorkUnits(this, docletEnv, reporter, docletEnv.getIncludedElements());

        final Set<String> uniqueGroups = new HashSet<>();


lbergelson · 2022-10-18T19:27:41Z

src/main/java/org/broadinstitute/barclay/help/HelpDoclet.java


+    @Override
+    public Set<? extends Option> getSupportedOptions() {
+        return new HashSet<>() {{


lbergelson · 2022-10-18T19:27:49Z

src/main/java/org/broadinstitute/barclay/help/HelpDoclet.java

-     */
-    protected void validateDocletStartingState() {
+    private DocletEnvironment docletEnv;      // The javadoc doclet env
+    protected Set<DocWorkUnit> workUnits = new HashSet<>();     // Set of all things we are going to document


lbergelson · 2022-10-18T20:01:38Z

src/main/java/org/broadinstitute/barclay/help/scanners/DocumentedFeatureScanner.java

+ * An {@link ElementScanner14} for finding {@link DocumentedFeature}s to be included in a Barclay doc
+ * task. Returns a set of {@link DocWorkUnit} objects.
+ */
+public class DocumentedFeatureScanner extends ElementScanner14<Void, Void> {


Shouldn't this be a scanner 17?

Yes, but ElementScanner17 doesn't appear to exist. ElementScanner14 is annotated with (@SupportedSourceVersion(RELEASE_17). I assume they do this when the new version has no language changes that would warrant a new class, but the doc for these classes is pretty minimal.

That makes sense. I'm surprised 17 didn't have any new elements with the new pattern matching stuff.

lbergelson · 2022-10-18T20:02:07Z

src/main/java/org/broadinstitute/barclay/help/scanners/FieldScanner.java

+ * element, even if the field is declared within the class represented by the (enclosing) element, i.e.,
+ * this can happen if the field is initialized by instantiating an anonymous class.
+ */
+public class FieldScanner extends ElementScanner14<Void, Void> {


Same question about making this a scanner 17.

See the other comments.

cmnbroad · 2022-10-25T13:04:53Z

src/main/java/org/broadinstitute/barclay/help/WDLDoclet.java

+//     */
+//    public static boolean run(final DocletEnvironment docEnv) throws IOException {
+//        return super.start;
+//    }


Oh, this should be removed entirely and just rely on the base class.

… unit tests, fix custom tags.

…elements only, use canonical class name.

cmnbroad · 2023-01-24T22:21:05Z

@lbergelson I think this is ready for review now. I updated a lot of code based on things I found when running the GATK docgen with this code, and comparing the results to the current code generated on GATK master. Sorry for the churn.

lbergelson

@cmnbroad This looks good to me 👍

lbergelson · 2023-01-25T22:02:32Z

src/main/java/org/broadinstitute/barclay/help/DefaultDocWorkUnitHandler.java

        if (fieldElement == null) {
-            return "";
+            // the field isn't defined directly in the workunit's enclosing class/element, so it must be


These comments are extremely helpful.

cmnbroad force-pushed the cn_java_upgrade branch from 0f63959 to 57851aa Compare October 11, 2022 15:05

cmnbroad mentioned this pull request Oct 11, 2022

Upgrade to gradle 7.4.2. #186

Closed

cmnbroad force-pushed the cn_java_upgrade branch 2 times, most recently from 52055a0 to c2d3267 Compare October 17, 2022 14:50

lbergelson requested changes Oct 18, 2022

View reviewed changes

cmnbroad commented Oct 25, 2022

View reviewed changes

cmnbroad mentioned this pull request Nov 7, 2022

[Debian] Switching to OpenJDK 17: issues #185

Open

Upgrade to Java 17 and gradle 7.4.2; use new doclet APIs; remove mock…

bc936c0

… unit tests, fix custom tags.

cmnbroad force-pushed the cn_java_upgrade branch from c2d3267 to 7a858a9 Compare December 19, 2022 20:35

Review comments.

5cf14fd

cmnbroad force-pushed the cn_java_upgrade branch from d6a1bd7 to cb4da7a Compare January 3, 2023 22:06

cmnbroad force-pushed the cn_java_upgrade branch from cb4da7a to 538c64c Compare January 23, 2023 14:22

Fix fixes based on testing, fix javadoc process arts, use FIELD kind …

86ba9c1

…elements only, use canonical class name.

cmnbroad force-pushed the cn_java_upgrade branch from 538c64c to 86ba9c1 Compare January 24, 2023 19:05

lbergelson approved these changes Jan 25, 2023

View reviewed changes

cmnbroad merged commit d4c85aa into master Jan 31, 2023

cmnbroad deleted the cn_java_upgrade branch January 31, 2023 17:03

cmnbroad mentioned this pull request Feb 14, 2023

Java 17 and Spark 3.3.0. broadinstitute/gatk#8035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Java 17 and gradle 7.4.2. #188

Upgrade to Java 17 and gradle 7.4.2. #188

cmnbroad commented Oct 11, 2022 •

edited

Loading

codecov bot commented Oct 11, 2022 •

edited

Loading

lbergelson left a comment

lbergelson Oct 17, 2022

lbergelson Oct 17, 2022

cmnbroad Oct 25, 2022

lbergelson Oct 17, 2022

lbergelson Oct 18, 2022

lbergelson Oct 18, 2022

cmnbroad Oct 25, 2022

lbergelson Oct 18, 2022

lbergelson Oct 18, 2022

cmnbroad Oct 25, 2022

cmnbroad Oct 25, 2022

lbergelson Oct 18, 2022

cmnbroad Oct 25, 2022

lbergelson Oct 18, 2022

cmnbroad Oct 25, 2022

lbergelson Oct 18, 2022

cmnbroad Oct 25, 2022

lbergelson Oct 18, 2022

cmnbroad Oct 25, 2022

lbergelson Oct 25, 2022

lbergelson Oct 18, 2022

cmnbroad Oct 25, 2022

cmnbroad Oct 25, 2022

cmnbroad Oct 25, 2022

cmnbroad commented Jan 24, 2023

lbergelson left a comment

lbergelson Jan 25, 2023

Upgrade to Java 17 and gradle 7.4.2. #188

Upgrade to Java 17 and gradle 7.4.2. #188

Conversation

cmnbroad commented Oct 11, 2022 • edited Loading

codecov bot commented Oct 11, 2022 • edited Loading

Codecov Report

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad commented Jan 24, 2023

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad commented Oct 11, 2022 •

edited

Loading

codecov bot commented Oct 11, 2022 •

edited

Loading