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

Plugin breaks aar module #523

Closed
matejdro opened this issue Aug 19, 2021 · 14 comments · Fixed by #571
Closed

Plugin breaks aar module #523

matejdro opened this issue Aug 19, 2021 · 14 comments · Fixed by #571

Comments

@matejdro
Copy link

When ktlint plugin is applied to a module that just holds AAR artifact (via allprojects block), it appears to break that module's output - build complains about classes from that module missing.

Steps to reproduce:

  1. Download and assemble KtlintAar.zip
  2. Note that build will fail
  3. Go to root build.gradle and comment out apply plugin line for ktlint
  4. Assemble again
  5. Build will work
@matejdro
Copy link
Author

It appears this started happening with 10.1.0. Workaround for the issue is downgrading to 10.0.0

@JLLeitschuh
Copy link
Owner

Can you provide a build scan and and the output of the error you see?

@matejdro
Copy link
Author

matejdro commented Sep 8, 2021

Error message has nothing directly to do with the plugin, it just complains that classes are missing:

> Task :app:compileDebugKotlin FAILED
e: /home/matej/Downloads/KtlintAarOut/app/src/main/java/com/matejdro/permissions/ktlintaar/MainActivity.kt: (5, 33): Unresolved reference: testlibrarysource
e: /home/matej/Downloads/KtlintAarOut/app/src/main/java/com/matejdro/permissions/ktlintaar/MainActivity.kt: (11, 9): Unresolved reference: Test

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:compileDebugKotlin'.
> Compilation error. See log for more details

Here is the scan: https://gradle.com/s/aqt5juhhezx4u

Sorry to be so blunt, but I have attached repro project above, why was the build scan also necessary? Are you having troubles reproducing it from the attached sample?

@JLLeitschuh
Copy link
Owner

Sorry to be so blunt, but I have attached repro project above, why was the build scan also necessary? Are you having troubles reproducing it from the attached sample?

I generally try to avoid running untrusted code from maintainers I'm not familiar with. Nothing personal, I'm just being security conscious.

Personally, I'm not familiar with aar files and how Gradle nor the android plugin handles them.

As far as I'm aware this plugin doesn't modify any classpaths other than the ones that it creates for itself. I don't understand how this plugin could impact the success/failure of another plugin's compilation. @Tapchicoma thoughts here?

@matejdro
Copy link
Author

matejdro commented Sep 8, 2021

Feel free to replace all my gradle distribution files with your own, so you are not running anything untrusted.

@juandallaglio
Copy link

juandallaglio commented Oct 4, 2021

I'm having the same problem , could you find the root issue causing it?

Thanks!

@tinkooladik
Copy link

I had a similar issue, maybe this will help someone.

In my case we have dependency on additional java module, added to the project. So I manually excluded that java module from adding ktlint stuff.

subprojects {
    // we should exclude java modules, otherwise build fails
    if (it.name != 'java-module-name') {
        apply plugin: "org.jlleitschuh.gradle.ktlint"
        
        ......
    }
}

I think with aars it should work the same.

@ninniuz
Copy link

ninniuz commented Jan 25, 2022

I am having the same issue and I think I was able to bypass it with the following

configurations.maybeCreate("default").apply {
    attributes {
        attribute(
            TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE,
            objects.named(TargetJvmEnvironment::class.java, TargetJvmEnvironment.ANDROID)
        )
    }
}

From what I can see from dependency insights, this line is making ktlint variant selected instead of the default configuration in my case.
With the above snippet I am guiding the variant selection as needed.

@tbroyer
Copy link
Contributor

tbroyer commented Jan 25, 2022

@JLLeitschuh @Tapchicoma AIUI, the ktlint configuration is being exported by the projects, and participates and variant-aware resolution when that project is depended upon by another project.
Could the solution be as easy as configuring the configuration as isCanBeConsumed = false?

@ninniuz
Copy link

ninniuz commented Jan 25, 2022

@tbroyer I tried your solution on the consuming side (that is, on my module applying the ktlint plugin)

configurations.named("ktlint") {
    isCanBeConsumed = false
}

and that seems to work as well

@JLLeitschuh
Copy link
Owner

I'm happy to merge any PRs that fix this issue with tests demonstrating the bug and fixing it.

I have to admit, I don't actually fully understand the problem here or why it's occurring. I don't why this is causing issues:

On the other end, at the library project side (the producer), we also use configurations to represent what can be consumed. For example, the library may expose an API or a runtime, and we would attach artifacts to either one, the other, or both. Typically, to compile against lib, we need the API of lib, but we don’t need its runtime dependencies. So the lib project will expose an apiElements configuration, which is aimed at consumers looking for its API. Such a configuration is consumable, but is not meant to be resolved. This is expressed via the canBeConsumed flag of a Configuration:
- https://docs.gradle.org/current/userguide/declaring_dependencies.html#:~:text=On%20the%20other,of%20a%20Configuration%3A

What is consuming this configuration other than the KtLint plugin. If something else is consuming the ktlint configuration, that sounds like a bug in the downstream plugin, not in this plugin directly. But again, I may be completely misunderstanding the issue.

@tbroyer
Copy link
Contributor

tbroyer commented Jan 26, 2022

Let's look at it the other way around: is there any reason for the ktlint configuration to be consumable by other projects? The KtLint plugin will resolve it in the same project, but will never need to consume it from another project. IMO the configuration should be isCanBeConsumed = false and isVisible = false.

Here's a small reproducer:

// settings.gradle.kts
include("producer", "consumer")
// producer/build.gradle.kts
plugins {
  id("org.jlleitschuh.gradle.ktlint") version "10.1.0"
}
configurations.create("default")
artifacts.add("default", buildFile)
// consumer/build.gradle.kts
plugins {
  `java-library`
}
dependencies {
  implementation(project(":consumer"))
}

Now run gradle :consumer:dependencies and/or gradle :consumer:dependencyInsight --configuration compileClasspath --dependency producer.
You can also start poking around: comment out the KtLint plugin, add the java-base plugin rather than creating the default configuration manually, add the java plugin, add an attribute to the default configuration (any attribute that's requested by consumer, as shown by dependencyInsight), etc. Depending on the changes, you'll either see the variant switch from ktlint to default or an error because Gradle cannot determine which variant to use. Notice how the behavior changes depending on the attribute(s) you add to the default configuration.

What seems to happen here is that the default configuration above is created without attribute, so when the project is consumed from the other project, Gradle has to decide between a variant without attribute and another with one matching/compatible attribute (ktlint), so it chooses the latter.
When you change the default configuration to add one matching attribute, Gradle cannot decide and the build fails.
If you add the bundling attribute and another matching attribute, Gradle will prefer the default configuration over the ktlint one.
When you use a previous version of the KtLint plugin, that didn't declare any attribute on the ktlint configuration, then Gradle chooses the default configuration because it cannot use variant-aware resolution at all. The addition of the attribute on the ktlint configuration that is consumable broke the projects because then it "enabled" variant-aware resolution, and ktlint is a valid variant for the project.

You could argue that the producer project should declare attributes on its default configuration, but I'd say the ktlint configuration has no reason to be consumable to begin with.

I tested, and adding:

isCanBeResolved = true
isCanBeConsumed = false
isVisible = false

to the ktlint configuration fixes the problem.
Ideally, this should be added to all the KtLint-related configurations, as there's no reason that the ruleset, baseline, and reporters be consumable as well.
(I explicitly set isCanBeResolved = true despite that being the default value, for exhaustiveness and explicitness)

@tbroyer
Copy link
Contributor

tbroyer commented Jan 27, 2022

Fwiw (emphasis mine):

In short, a configuration’s role is determined by the canBeResolved and canBeConsumed flag combinations:

Configuration role can be resolved can be consumed
Bucket of dependencies false false
Resolve for certain usage true false
Exposed to consumers false true
Legacy, don’t use true true

For backwards compatibility, both flags have a default value of true, but as a plugin author, you should always determine the right values for those flags, or you might accidentally introduce resolution errors.

https://docs.gradle.org/current/userguide/declaring_dependencies.html#:~:text=In%20short%2C%20a,introduce%20resolution%20errors.

tbroyer added a commit to tbroyer/ktlint-gradle that referenced this issue Jan 27, 2022
Keeping the default of being consumable exposes them to
variant-aware resolution when the project is depended
upon from another project.

Also configure the configurations as non-visible for
similar reasons: those are specific to this project's
build and there's no reason to consume them from
another project.

Fix JLLeitschuh#523
@JLLeitschuh
Copy link
Owner

Excelent description!

JLLeitschuh pushed a commit that referenced this issue Feb 7, 2022
Keeping the default of being consumable exposes them to
variant-aware resolution when the project is depended
upon from another project.

Also configure the configurations as non-visible for
similar reasons: those are specific to this project's
build and there's no reason to consume them from
another project.

Fix #523
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 a pull request may close this issue.

6 participants