Mark Missing Local Artifacts as optional #23245
Conversation
|
@bot-gradle test this |
|
OK, I've already triggered the following builds for you: |
|
@bot-gradle test this |
|
I've triggered the following builds for you: |
|
|
||
| @Override | ||
| public boolean isOptionalArtifact() { | ||
| return true; |
There was a problem hiding this comment.
This is not a method I am super familiar with.
Can you explain why a blanket change like this to all instances of MissingLocalArtifactMetadata is not an issue?
And looks like we have tests that fail because of the change.
There was a problem hiding this comment.
So my previous test run succeeded here, then my only change after was to add a new integration test. So that's a little strange.
I was looking for failing tests and when I didn't see any assumed we could mark missing but local metadata artifacts as optional so that we don't throw an exception when trying to get() them. I'll look a little deeper into these now failing tests, maybe they can throw some new light on this.
|
Now I'm wondering if the behavior in 7.5 was correct. Maybe this ought to have been failing there as well. |
| import org.gradle.integtests.fixtures.AbstractIntegrationSpec | ||
| import spock.lang.Issue | ||
|
|
||
| @Issue("https://github.com/gradle/gradle/pull/23245") |
There was a problem hiding this comment.
I think you meant to link to the issue here?
|
So we're no longer honoring this comment currently because of how we now attempt to get the lazily resolved artifacts prior to visiting them with a build operation. |
| artifacts = artifactsSupplier.get(); | ||
| } catch (Exception e) { | ||
| return EMPTY; | ||
| } | ||
| if (artifacts.isEmpty()) { |
There was a problem hiding this comment.
If there was an easy way to pass information about whether we are resolving leniently or strictly to the ArtifactBackedResolvedVariant#create method as it calls supplyResolvedArtifacts so that we can add this (or similar error handling) when this supplier's get() is called, this would be a straightforward change, I think.
Alternately, if we could change the Supplier to a Function and pass in the configuration doing the resolving when get()/call() is called, we could do the same thing.
As it is now, passing this information all the way into this Supplier when it is created seems like it would require sending a flag down a very long chain of resolution calls.
|
OBE #23399 |
Fixes: #22875
Added an integration test that replicates the issue.