gradle / gradle

Adaptable, fast automation for all
https://gradle.org
Apache License 2.0
17.02k stars 4.78k forks source link

Attributes on an artifact view are always treated leniently #27773

Open Vampire opened 10 months ago

Vampire commented 10 months ago

Current Behavior

If you specify attributes on an artifact view to select a specific artifact, or even to select a different variant using withVariantReselection(), you cannot make the artifact view fail if there is a resolution error, but it is always treated leninently.

Expected Behavior

The lenient property on the artifact view should control whehter an artifact view behaves lenient or strict.

Steps to Reproduce

Here a reproducer with three variants to comment in / out.

If you execute some task with "(1)", it fails as expected as the attribute cannot be satisfied. If you execute some task with "(2)", it works, producing no output as the artifact view is lenient.

Now the unexpected, if you execute some task with "(3)", it works producing no output, but I would have exepcted it to fail. I did not switch the artifact view to lenient - even for the sake of the example set it to false explicitly - so it should fail when trying to resolve and not finding a matching variant.

Even when not using withVariantReselection() but just using attributes to select an artifact in the view, it should not behave lenient always.

Even if lenient would stay the default, it should at least be possible to switch to strict mode.

repositories {
    gradlePluginPortal()
}
val foo by configurations.dependencyScope("foo")
dependencies {
    components.all { allVariants { attributes.attribute(TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE, objects.named("foo")) } }
    foo("com.gradle.enterprise:com.gradle.enterprise.gradle.plugin:3.16.1")
}
val bar by configurations.resolvable("bar") {
    extendsFrom(foo)
    attributes {
        attribute(TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE, objects.named(TargetJvmEnvironment.ANDROID))
    }
}
val baz by configurations.resolvable("baz") {
    extendsFrom(foo)
}
// (1) failed
//bar.forEach { println("FOO: $it") }
// (2) geht
//bar.incoming.artifactView { isLenient = true }.files.forEach { println("FOO: $it") }
// (3) geht, sollte aber nicht
baz.incoming.artifactView {
    isLenient = false
    withVariantReselection()
    attributes {
        attribute(TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE, objects.named(TargetJvmEnvironment.ANDROID))
    }
}.files.forEach { println("FOO: $it") }

Gradle version

8.5

cobexer commented 10 months ago

Thank you for providing a valid report.

The issue is in the backlog of the relevant team and is prioritized by them.

jvandort commented 3 months ago

I agree the existing behavior is unexpected. I also don't think it is the right behavior.

If you change the attributes of an artifact view, we allow the artifact view to fail to select artifacts. I'm not sure why this was originally chosen but it seems like the wrong behavior.

However, I wonder how we could go about improving this.

Perhaps we could emit a deprecation warning if we failed to select artifacts without the user setting some flag.

I don't think that flag should be lenient though, as that makes us lenient to a lot more errors, such as failing to download artifacts.

Vampire commented 3 months ago

Lenient mode ignores download errors? 😱 That's actually also very unexpected to me and sounds like a very bad idea. That means a build behaves differently depending on network availability. How is that anything near to reproducible, deterministic, and reliable builds? Or did you only mean 404?

jvandort commented 3 months ago

AFAIK lenient ignores just about any error that could occur during artifact selection. We should probably restrict this to strictly unresolved dependencies or 404s, but ATM I think just about any exception is ignored.

I agree we should change that behavior as well, however that is technically a different "switch" than what you are requesting in this issue.

In particular, LenientConfiguration's "lenient" is a bit different, and probably what we should be aiming more towards here, where lenient there means (AFAIK) "You asked for a dependency and it didnt exist, or I tried to download a jar and it didnt exist even though I found the POM". Lenient in ArtifactView is pretty much a try/catch around artifact selection/resolution.

Vampire commented 3 months ago

Wasn't my point that the artifact view behaves lenient, while it is not configured to be lenient?

jvandort commented 3 months ago

Lenient can be viewed in multiple ways:

  1. When selecting artifacts with attribute matching, we do not find a variant with those attributes. This is what this issue is talking about. When you specify attributes on an artifact view, we allow variant matching to fail to find a matching variant.
  2. When resolving the original graph we failed to find the metadata for the dependency (no POM, the dependency doesn't exist at all in the target repo)
  3. When selecting artifacts with attribute matching, we succeed to find a matching variant, but then when downloading the file that the variant points to, we get a 401, 404, 500, etc.
  4. When selecting artifacts with attribute matching, we succeed to find a matching variant, but then when downloading the file that the variant points to, we get some exception -- maybe the connection closes unexpectedly, the connection is refused, maybe some DNS error, or a network connection is unavailable
jvandort commented 3 months ago

I was just pointing out that specifying attributes on an artifact view is only "lenient" in the first case. 2-4 will still throw an exception.

Not to say it should be lenient in the first case, but it is different than the lenient flag on an artifact view

jvandort commented 3 months ago

Thankfully, I have confirmed that dependency verification failures are fatal with lenient artifact views

jvandort commented 3 months ago

After further testing, it seems a lenient artifact view and a LenientConfiguration behave similarly in terms of the types of failures they permit

Vampire commented 3 months ago

So what does the lenient flag on the artifact view then do currently? Allowing 2-4 while 1 is always allowed?

jvandort commented 3 months ago

Lenient allows 2-4 (though should probably not allow 4). 1 is only allowed when you explicitly specify attributes on the artifact view (but 1 should instead be configurable)

jvandort commented 3 months ago

Usually when 4 happens 2 would also happen (more like 2.5)

Vampire commented 3 months ago

Ah, now I understand, thanks. So basically there are two issues here. Make 1 configurable and better document what the effect of lenient is. :-)

jvandort commented 3 months ago

Yes that seems about right

Are your concerns from here mostly alleviated, assuming we work these two issues? https://github.com/gradle/gradle/issues/27773#issuecomment-2275076090

Vampire commented 3 months ago

Well, if it is clear what it does, that might be fine. My main concern there probably stemmed from the fact that I thought lenient should make the artifact view lenient as in not failing if matching algorithm didn't find anything.

Now that I know that lenient means "ignore any errors at all just like doing a try-catch", that is fine if one is aware of that. There might be very few but some use-cases where this might be helpful, like "download all sources but don't care if anything goes wrong, it is fine if they are not there in the end even if they could be without network error". If one is aware of that and does not use lenient mode in most cases, but the new-to-be-defined option to toggle failure on match algorithm failure only, then it is probably ok.

jvandort commented 3 months ago

This is an issue we would like to work "relatively" soon. Depending on when we pick it up, likely early in 9.x or perhaps late in 8.x. No promises here.

Though, if you want to open an issue for the poor "lenient" documentation, we can likely work that one earlier

Vampire commented 3 months ago

Sure: #30314 :-)