Generate accessors for `ArtifactHandler` #1020

mkobit · 2018-08-08T17:12:03Z

issue #889

Signed-off-by: Mike Kobit [email protected]

Context

#889

Contributor Checklist

Base the PR against the develop branch
Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
Provide integration tests to verify changes from a user perspective
Provide unit tests to verify logic
Ensure that tests pass locally: ./gradlew check --parallel

mkobit · 2018-08-08T19:07:28Z

This doesn't entirely address all use cases but hits some of them. For example, something like

val myConfig by configurations.creating
artifacts {
    myConfig(something)
}

Wouldn't work (right now).

Want me to tackle that in this MR or a different one? What are the desired supported DSL invocations?

JLLeitschuh · 2018-10-17T23:30:34Z

@mkobit Any chance this can be updated so that it can be merged?

mkobit · 2018-10-19T14:57:09Z

@JLLeitschuh I was going to wait for some feedback before I spent more time on this. I wasn't sure if the direction going forward is to use myConfiguration.outgoing.artifact(someThing) instead of the artifacts {} block. I'm in favor of reducing the amount of ways to do something, so for me I'd like to see clarification around that before I add more here. I would even be in favor of deprecation of ArtifactHandler.

eskatos · 2018-11-06T13:26:34Z

@mkobit, the target code changed a lot, would you be willing to rebase on top of develop and fix the conflicts? the added coverage shouldn't cause trouble

mkobit · 2018-11-06T15:17:08Z

@eskatos yes, I'll try and tackle it in the next couple of days

mkobit · 2018-11-19T17:29:26Z

I'm trying to work through this now, but converting it to the AccessorFragments byte code is proving to be more challenging for me than I would like. I'm basically cargo-culting what you guys have done for DependencyHandler while also trying to strip down output from a javap -c -p -verbose SomeClass.class, where SomeClass.class is coming from the copy/pasted logic I have added into buildSrc since it seems easier for me to find the byte code.

I have, for the non-Action consuming method:

AccessorFragment(
    source = name.run {
      """
                  /**
                   * Adds an artifact to the '$original' configuration.
                   *
                   * @param artifactNotation the group of the module to be added as a dependency.
                   * @return The artifact.
                   *
                   * @see [ArtifactHandler.add]
                   */
                  fun ArtifactHandler.`$kotlinIdentifier`(artifactNotation: Any): PublishArtifact =
                      add("$stringLiteral", artifactNotation)
              """
    },
    bytecode = {
      publicStaticMethod(signature) {
        ALOAD(0)
        LDC(name.original)
        ALOAD(1)
        INVOKEINTERFACE(GradleTypeName.artifactHandler, "add", "(Ljava/lang/String;Ljava/lang/Object;)Lorg/gradle/api/artifacts/PublishArtifact;")
        ARETURN()
      }
    },
    metadata = {
      writer.writeFunctionOf(
          receiverType = GradleType.artifactHandler,
          returnType = GradleType.publishArtifact,
          name = propertyName,
          functionFlags = publicFunctionFlags,
          parameters = {
            visitParameter("artifactNotation", KotlinType.any)
          },
          signature = signature
      )
    },
    signature = JvmMethodSignature(
        name.original,
        "(Lorg/gradle/api/artifacts/dsl/ArtifactHandler;Ljava/lang/Object;)Lorg/gradle/api/artifacts/PublishArtifact;"
    )
)

and testing that seems to work.

Then, for the other method, I am obviously missing something:

AccessorFragment(
    source = name.run {
      """
                    /**
                     * Adds an artifact to the '$original' configuration.
                     *
                     * @param artifactNotation the group of the module to be added as a dependency.
                     * @param configureAction The action to execute to configure the artifact.
                     * @return The artifact.
                     *
                     * @see [ArtifactHandler.add]
                     */
                    fun ArtifactHandler.`$kotlinIdentifier`(
                        artifactNotation: Any,
                        configureAction:  ConfigurablePublishArtifact.() -> Unit): PublishArtifact =
                            add("$stringLiteral", artifactNotation, configureAction)
                """
    },
    bytecode = {
      publicStaticMethod(signature) {
        ALOAD(0)
        LDC(propertyName)
        ALOAD(1)
        ALOAD(2)
        INVOKEINTERFACE(GradleTypeName.artifactHandler, "add", "(Ljava/lang/String;Ljava/lang/Object;Lorg/gradle/api/Action;)Lorg/gradle/api/artifacts/PublishArtifact;")
        ARETURN()
      }
    },
    metadata = {
      writer.writeFunctionOf(
          receiverType = GradleType.artifactHandler,
          returnType = GradleType.publishArtifact,
          name = propertyName,
          functionFlags = publicFunctionFlags,
          parameters = {
            visitParameter("artifactNotation", KotlinType.any)
            visitParameter("configureAction", actionTypeOf(GradleType.configurablePublishArtifact))
          },
          signature = signature
      )
    },
    signature = JvmMethodSignature(
        name.original,
        "(Lorg/gradle/api/artifacts/dsl/ArtifactHandler;Ljava/lang/Object;Lkotlin/jvm/functions/Function1<-Lorg/gradle/api/artifacts/ConfigurablePublishArtifact;Lkotlin/Unit;>;)Lorg/gradle/api/artifacts/PublishArtifact;"
    )
)

leads to

Caused by: java.lang.ArrayIndexOutOfBoundsException: 210
	at org.jetbrains.org.objectweb.asm.Type.getArgumentTypes(Type.java:343)
	at org.jetbrains.org.objectweb.asm.MethodWriter.visitMaxs(MethodWriter.java:1464)
	at org.gradle.kotlin.dsl.support.bytecode.AsmExtensionsKt.method(AsmExtensions.kt:118)
	at org.gradle.kotlin.dsl.support.bytecode.AsmExtensionsKt.publicStaticMethod(AsmExtensions.kt:90)
	at org.gradle.kotlin.dsl.support.bytecode.KotlinMetadataKt.publicStaticMethod(KotlinMetadata.kt:96)
	at org.gradle.kotlin.dsl.support.bytecode.KotlinMetadataKt.publicStaticMethod$default(KotlinMetadata.kt:93)
	at org.gradle.kotlin.dsl.accessors.AccessorFragmentsKt$fragmentsForConfiguration$1$23.invoke(AccessorFragments.kt:454)
	at org.gradle.kotlin.dsl.accessors.AccessorFragmentsKt$fragmentsForConfiguration$1$23.invoke(AccessorFragments.kt)
	at org.gradle.kotlin.dsl.accessors.EmitterKt.emitClassFor(Emitter.kt:78)
	at org.gradle.kotlin.dsl.accessors.EmitterKt.access$emitClassFor(Emitter.kt:1)
	at org.gradle.kotlin.dsl.accessors.EmitterKt$emitAccessorsFor$emittedClassNames$1.invoke(Emitter.kt:47)
	at org.gradle.kotlin.dsl.accessors.EmitterKt$emitAccessorsFor$emittedClassNames$1.invoke(Emitter.kt)
	at kotlin.sequences.TransformingSequence$iterator$1.next(Sequences.kt:174)
	at kotlin.sequences.SequencesKt___SequencesKt.toCollection(_Sequences.kt:691)
	at kotlin.sequences.SequencesKt___SequencesKt.toMutableList(_Sequences.kt:721)
	at kotlin.sequences.SequencesKt___SequencesKt.toList(_Sequences.kt:712)
	at org.gradle.kotlin.dsl.accessors.EmitterKt.emitAccessorsFor(Emitter.kt:48)
	at org.gradle.kotlin.dsl.accessors.AccessorsClassPathKt.buildAccessorsFor(AccessorsClassPath.kt:147)
	at org.gradle.kotlin.dsl.accessors.AccessorsClassPathKt$buildAccessorsClassPathFor$$inlined$let$lambda$1.invoke(AccessorsClassPath.kt:68)
	at org.gradle.kotlin.dsl.accessors.AccessorsClassPathKt$buildAccessorsClassPathFor$$inlined$let$lambda$1.invoke(AccessorsClassPath.kt)
	at org.gradle.kotlin.dsl.accessors.AccessorsClassPathKt$cachedAccessorsClassPathFor$cacheDir$1.invoke(AccessorsClassPath.kt:97)
	at org.gradle.kotlin.dsl.accessors.AccessorsClassPathKt$cachedAccessorsClassPathFor$cacheDir$1.invoke(AccessorsClassPath.kt)
	at org.gradle.kotlin.dsl.cache.ScriptCache.initializeCacheDir(ScriptCache.kt:113)
	at org.gradle.kotlin.dsl.cache.ScriptCache.access$initializeCacheDir(ScriptCache.kt:37)
	at org.gradle.kotlin.dsl.cache.ScriptCache$cacheDirFor$1.execute(ScriptCache.kt:60)
	at org.gradle.kotlin.dsl.cache.ScriptCache$cacheDirFor$1.execute(ScriptCache.kt:37)
	at org.gradle.cache.internal.DefaultPersistentDirectoryCache$Initializer.initialize(DefaultPersistentDirectoryCache.java:98)
	at org.gradle.cache.internal.FixedSharedModeCrossProcessCacheAccess$1.run(FixedSharedModeCrossProcessCacheAccess.java:85)
	at org.gradle.cache.internal.DefaultFileLockManager$DefaultFileLock.doWriteAction(DefaultFileLockManager.java:207)
	at org.gradle.cache.internal.DefaultFileLockManager$DefaultFileLock.writeFile(DefaultFileLockManager.java:197)
	at org.gradle.cache.internal.FixedSharedModeCrossProcessCacheAccess.open(FixedSharedModeCrossProcessCacheAccess.java:83)
	at org.gradle.cache.internal.DefaultCacheAccess.open(DefaultCacheAccess.java:141)
	at org.gradle.cache.internal.DefaultPersistentDirectoryStore.open(DefaultPersistentDirectoryStore.java:78)

Any suggestions for working through this?

bamboo · 2018-11-19T17:34:17Z

@mkobit my workflow was to write the Kotlin code I wanted to generate around this code and use the Show Kotlin Bytecode IntelliJ command on it.

issue #889 Signed-off-by: Mike Kobit <[email protected]>

… {}` DSL block Signed-off-by: Mike Kobit <[email protected]>

mkobit · 2018-11-19T23:28:39Z

Thanks for the tip @bamboo - I haven't dove deeply into JVM byte code operations before so I definitely learned a few things while working on this.

I think this is in a ready-for-review state now.

JLLeitschuh · 2018-11-20T02:23:26Z

subprojects/provider/src/main/kotlin/org/gradle/kotlin/dsl/ArtifactHandlerScope.kt

+    /**
+     * Configures the artifacts.
+     */
+    inline operator fun invoke(configuration: ArtifactHandlerScope.() -> Unit) =


Does this return the PublishArtifact?

Returns Unit like the DependencyHandlerScope - I followed the same style that was there, but would be alright with marking the return as explicit

Let's keep things uniform. If you feel strongly about it @JLLeitschuh, please open a separate issue with a motivation keeping api/dsl uniformity in mind.

I was just checking, I couldn't tell from reading the code what was returned. I don't feel strongly either way.

eskatos

First review pass, I'll get back to this again later

...s/src/test/kotlin/org/gradle/kotlin/dsl/integration/ProjectSchemaAccessorsIntegrationTest.kt

subprojects/provider/src/main/kotlin/org/gradle/kotlin/dsl/accessors/AccessorsClassPath.kt

subprojects/provider/src/main/kotlin/org/gradle/kotlin/dsl/accessors/CodeGenerator.kt

...s/src/test/kotlin/org/gradle/kotlin/dsl/integration/ProjectSchemaAccessorsIntegrationTest.kt

eskatos · 2018-11-22T17:01:15Z

subprojects/provider/src/main/kotlin/org/gradle/kotlin/dsl/ArtifactHandlerScope.kt

+    /**
+     * Configures the artifacts.
+     */
+    inline operator fun invoke(configuration: ArtifactHandlerScope.() -> Unit) =


Let's keep things uniform. If you feel strongly about it @JLLeitschuh, please open a separate issue with a motivation keeping api/dsl uniformity in mind.

JLLeitschuh · 2018-11-27T16:36:51Z

@mkobit DCO is failing on these, you may need to use an interactive rebase to add the sign-off on some of these commits.

mkobit · 2018-11-27T16:55:28Z

Thanks @JLLeitschuh - I guess I need a way to automate that...

Signed-off-by: Mike Kobit <[email protected]>

eskatos

Thank you @mkobit!

eskatos added the a:feature label Aug 14, 2018

eskatos added in:kotlin-dsl-compilation in:kotlin-dsl-runtime re:ease-of-use re:groovy-parity labels Nov 6, 2018

eskatos added this to the 1.1.0 milestone Nov 6, 2018

mkobit added 2 commits November 19, 2018 14:51

Generate accessors for ArtifactHandler

3e65c79

issue #889 Signed-off-by: Mike Kobit <[email protected]>

Add support for adding artifacts to a configuration in the `artifacts…

7ae96ee

… {}` DSL block Signed-off-by: Mike Kobit <[email protected]>

JLLeitschuh reviewed Nov 20, 2018

View reviewed changes

eskatos reviewed Nov 22, 2018

View reviewed changes

mkobit added 3 commits November 27, 2018 11:16

Remove accidental indentation

9b723f8

Signed-off-by: Mike Kobit <[email protected]>

Polish artifacts accessors integration test

ca51d26

Signed-off-by: Mike Kobit <[email protected]>

Remove excessive blank line

afeb9d2

Signed-off-by: Mike Kobit <[email protected]>

eskatos approved these changes Dec 5, 2018

View reviewed changes

eskatos merged commit 9046d5e into gradle:develop Dec 5, 2018

This was referenced Dec 5, 2018

Groovy parity: Generate Kotlin DSL accessors for ArtifactHandler #889

Closed

Refine ArtifactHandlerScope #1276

Merged

mkobit deleted the mk/889-generate-artifact-handler-accessors branch December 7, 2018 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate accessors for `ArtifactHandler` #1020

Generate accessors for `ArtifactHandler` #1020

mkobit commented Aug 8, 2018 •

edited

Loading

mkobit commented Aug 8, 2018

JLLeitschuh commented Oct 17, 2018

mkobit commented Oct 19, 2018

eskatos commented Nov 6, 2018

mkobit commented Nov 6, 2018

mkobit commented Nov 19, 2018 •

edited

Loading

bamboo commented Nov 19, 2018

mkobit commented Nov 19, 2018

JLLeitschuh Nov 20, 2018

mkobit Nov 20, 2018

eskatos Nov 22, 2018

JLLeitschuh Nov 23, 2018

eskatos left a comment

eskatos Nov 22, 2018

JLLeitschuh commented Nov 27, 2018

mkobit commented Nov 27, 2018

eskatos left a comment

Generate accessors for ArtifactHandler #1020

Generate accessors for ArtifactHandler #1020

Conversation

mkobit commented Aug 8, 2018 • edited Loading

Context

Contributor Checklist

mkobit commented Aug 8, 2018

JLLeitschuh commented Oct 17, 2018

mkobit commented Oct 19, 2018

eskatos commented Nov 6, 2018

mkobit commented Nov 6, 2018

mkobit commented Nov 19, 2018 • edited Loading

bamboo commented Nov 19, 2018

mkobit commented Nov 19, 2018

JLLeitschuh Nov 20, 2018

Choose a reason for hiding this comment

mkobit Nov 20, 2018

Choose a reason for hiding this comment

eskatos Nov 22, 2018

Choose a reason for hiding this comment

JLLeitschuh Nov 23, 2018

Choose a reason for hiding this comment

eskatos left a comment

Choose a reason for hiding this comment

eskatos Nov 22, 2018

Choose a reason for hiding this comment

JLLeitschuh commented Nov 27, 2018

mkobit commented Nov 27, 2018

eskatos left a comment

Choose a reason for hiding this comment

Generate accessors for `ArtifactHandler` #1020

Generate accessors for `ArtifactHandler` #1020

mkobit commented Aug 8, 2018 •

edited

Loading

mkobit commented Nov 19, 2018 •

edited

Loading