The conjure IR compiler supports standard customization #1049

carterkozak · 2022-05-17T18:41:10Z

Custom arguments are supported using the same mechanism as
generators, but using the parser key:

conjure {
  parser {
    verbose = true
  }
}

==COMMIT_MSG==
The conjure IR compiler supports standard customization using the parser key
==COMMIT_MSG==

Possible downsides?

parser isn't syntax we use elsewhere, however compiler and "IR" are not obvious to users, so I think parser is reasonable.

Custom arguments are supported using the same mechanism as generators, but using the `parser` key: ```groovy conjure { parser { verbose = true } } ```

changelog-app · 2022-05-17T18:41:13Z

Generate changelog in `changelog/@unreleased`

Type

Description

The conjure IR compiler supports standard customization using the parser key

Check the box to generate changelog(s)

Generate changelog entry

gradle-conjure-api/src/main/java/com/palantir/gradle/conjure/api/GeneratorOptions.java

carterkozak · 2022-05-17T18:44:19Z

gradle-conjure/src/main/java/com/palantir/gradle/conjure/CompileIrTask.java

@@ -88,6 +88,10 @@ public final File getOutputFile() {
    @Optional
    public abstract MapProperty<String, Serializable> getConjureExtensions();

+    @Input
+    @Optional
+    public abstract MapProperty<String, Object> getOptions();


In practice I suspect the places using GeneratorOptions as an input would benefit from MapProperty<String, Object>, but I didn't update existing consumers in this PR.

carterkozak · 2022-05-17T18:46:25Z

gradle-conjure/src/main/java/com/palantir/gradle/conjure/ConjurePlugin.java

        TaskProvider<GenerateConjureServiceDependenciesTask> serviceDependencyTask = project.getTasks()
                .named(ConjureBasePlugin.SERVICE_DEPENDENCIES_TASK, GenerateConjureServiceDependenciesTask.class);

-        ConjureExtension conjureExtension =
-                project.getExtensions().create(ConjureExtension.EXTENSION_NAME, ConjureExtension.class);
+        ConjureExtension conjureExtension = conjureBasePlugin.conjureExtension();


I moved the ConjureExtension registration to ConjureBasePlugin, and updated the interacting pieces to pass references from the plugin registration rather than digging through get/find-by-X. It seemed cleaner, but I wouldn't be surprised if it's an unspeakable sin against gradle.

Hmm, looks good to me but actually not sure why this isn't applied more often. I guess it requires a tighter coupling between plugins but that seems fine in this case. Maybe @CRogers has a stronger opinion here :)

I have done both. I don't know how I feel about one versus the other. The just access the plugin method feels nicer from a general-java-coding standpoint, everything in Gradle land does the latter though.

carterkozak · 2022-05-17T18:47:06Z

gradle-conjure/src/test/groovy/com/palantir/gradle/conjure/TestVersions.java

@@ -19,7 +19,7 @@
 public final class TestVersions {
    private TestVersions() {}

-    public static final String CONJURE = "4.11.2";
+    public static final String CONJURE = "4.30.0";


Updated so that the --verbose flag is actually used rather than listed as an unmatched input -- in practice it's not necessary for the test to pass, and we're only validating plumbing. I'll revert this out if it causes anything to fail.

CRogers · 2022-05-18T12:42:12Z

gradle-conjure-api/src/main/java/com/palantir/gradle/conjure/api/ConjureExtension.java

@@ -45,6 +46,11 @@ public final void python(@DelegatesTo(GeneratorOptions.class) Closure<GeneratorO
        closure.call();
    }

+    public final void parser(@DelegatesTo(GeneratorOptions.class) Closure<GeneratorOptions> closure) {


You should use Closure arguments as raw types in extensions for the gradle integration in intellij to work properly (without making a big warning box). Also, for Closure<V>, V is the return type so it shouldn't really be the delegate type.

Probably a nice thing to make an errorprone rule of actually

Huh, interesting! I copied from the other methods on this object, I'll update all of 'em.

I only just discovered this recently too!

fawind · 2022-05-18T12:46:50Z

gradle-conjure/src/main/java/com/palantir/gradle/conjure/ConjureBasePlugin.java

@@ -98,6 +116,9 @@ private static TaskProvider<CompileIrTask> createIrTasks(
            compileIr.getExecutableDir().set(extractCompilerTask.flatMap(ExtractExecutableTask::getOutputDirectory));
            compileIr.getOutputIrFile().set(irDir.map(dir -> dir.file(project.getName() + ".conjure.json")));
            compileIr.getProductDependencies().set(project.provider(pdepsExtension::getProductDependencies));
+            compileIr
+                    .getOptions()
+                    .set(project.provider(() -> conjureExtension.getParser().getProperties()));


Afaik, general best practice is to make all fields of an extension (i.e. conjureExtension) a property and that way you get the lazy wiring out of the box. But this works as well and don't think we need to refactor the extension now.

That makes sense, though making the change in this PR would result in either a large refactor, or inconsistency within the ConjureExtension -- what do you think?

Yeah, I would just leave it like this! Just wanted to flag but don't think its a big deal!

It definitely is nicer to just hook up the properties together directly, especially as Gradle can then work out the task dependencies automatically if there are files involved in outputs and inputs of different tasks. The one thing I will say is the extra providers makes the stacktrace more understandable when you have some horrible not-enough-laziness bug like I recently encountered with gradle-jdks, but I don't think that's a good enough reason to not just link them together (and you can always add these back when debugging).

I've filed #1050 to track

CRogers · 2022-05-18T12:59:45Z

gradle-conjure-api/src/main/java/com/palantir/gradle/conjure/api/ConjureExtension.java

    private final GeneratorOptions pythonOptions = new GeneratorOptions();
    private final Map<String, GeneratorOptions> genericOptions = new HashMap<>();

-    public final void typescript(@DelegatesTo(GeneratorOptions.class) Closure<GeneratorOptions> closure) {
+    public final <T> void typescript(@DelegatesTo(GeneratorOptions.class) Closure<T> closure) {


Hmm, I wonder if parameterising the value like this works in the gradle integration or it really does have to be a raw type like I've been doing.

Sorry, I misinterpreted your comment, my fault -- fixed

CRogers · 2022-05-18T13:00:12Z

👍

svc-autorelease · 2022-05-18T16:48:32Z

Released 5.27.0

The conjure IR compiler supports standard customization

15c1a81

Custom arguments are supported using the same mechanism as generators, but using the `parser` key: ```groovy conjure { parser { verbose = true } } ```

carterkozak requested review from CRogers and fawind May 17, 2022 18:41

Add generated changelog entries

3c50900

carterkozak commented May 17, 2022

View reviewed changes

gradle-conjure-api/src/main/java/com/palantir/gradle/conjure/api/GeneratorOptions.java Show resolved Hide resolved

carterkozak commented May 17, 2022

View reviewed changes

carterkozak added merge when ready autorelease labels May 17, 2022

CRogers reviewed May 18, 2022

View reviewed changes

fawind reviewed May 18, 2022

View reviewed changes

carterkozak added 2 commits May 18, 2022 08:51

ConjureExtension typevar

2905452

revapi

5fc31b0

CRogers reviewed May 18, 2022

View reviewed changes

CRogers removed the merge when ready label May 18, 2022

raw types

ba70ad3

carterkozak mentioned this pull request May 18, 2022

ConjureExtension should use Properties for fields for better task input attribution #1050

Open

carterkozak added the merge when ready label May 18, 2022

bulldozer-bot bot merged commit 3030cb4 into develop May 18, 2022

bulldozer-bot bot deleted the ckozak/support_parser_options branch May 18, 2022 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The conjure IR compiler supports standard customization #1049

The conjure IR compiler supports standard customization #1049

carterkozak commented May 17, 2022

changelog-app bot commented May 17, 2022 •

edited by carterkozak

Loading

carterkozak May 17, 2022

carterkozak May 17, 2022

fawind May 18, 2022

CRogers May 18, 2022

carterkozak May 17, 2022

CRogers May 18, 2022 •

edited

Loading

CRogers May 18, 2022

carterkozak May 18, 2022

CRogers May 18, 2022

fawind May 18, 2022

carterkozak May 18, 2022

fawind May 18, 2022

CRogers May 18, 2022

carterkozak May 18, 2022

CRogers May 18, 2022

carterkozak May 18, 2022

CRogers commented May 18, 2022

svc-autorelease commented May 18, 2022

The conjure IR compiler supports standard customization #1049

The conjure IR compiler supports standard customization #1049

Conversation

carterkozak commented May 17, 2022

Possible downsides?

changelog-app bot commented May 17, 2022 • edited by carterkozak Loading

Generate changelog in changelog/@unreleased

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

CRogers May 18, 2022 • edited Loading

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

CRogers commented May 18, 2022

svc-autorelease commented May 18, 2022

changelog-app bot commented May 17, 2022 •

edited by carterkozak

Loading

Generate changelog in `changelog/@unreleased`

CRogers May 18, 2022 •

edited

Loading