Docker: fix base image dependency inference for parametrized targets.

[kaos]

Fixes #20632

[kaos]

I marked it for picking to 2.19.x, as it's a low risk fix, but we can close that or hold it for later if we don't want it going into 2.19.1..

[huonw]

Thanks for the fix.

hold it for later if we don't want it going into 2.19.1.

This sounds good 👍

[huonw]

It looks like this was previously required, but is now optional. Is that an intentional change?

[kaos]

yes, I looked at

pants/src/rust/engine/address/src/lib.rs

Lines 45 to 46 in c15e54e

	rule address() -> AddressInput<'input>
	= path:path() target:target()? generated:generated()? parameters:parameters()? {

when re-modeling this to cover an actual valid address as close as possible. (perhaps being slightly more conservative with special characters in the various parts..)

[huonw]

Ah, okay. Given everything is now optional, I wonder if this means dependency inference will now be looking at ~every bit of text in a docker file, e.g. random text like abc matches the "optional path" bit (and, even an empty string now satisfies this regex, I think).

The change to allow inferring without a target name seems somewhat of separate to supporting parametrised targets, and is potentially more "dangerous"? For instance, being more likely to change user-observed behaviour by inferring spurious dependencies or performance issues.

Some options I can think of:

1. make the change here and cherrypick it all, as originally planned
2. make the change here but avoid cherrypicking
3. do the two changes separately, and cherry pick the support for parameters but not the optional-name one
4. something else?

Thoughts?

[kaos]

Astute observations, as always :)

I think, trying to recollect the original train of thought as best I can, could've been to be a little conservative with what values to pick up as potential addresses to infer as dependencies to, as back then the "try this value and see if it resolves to something" was a little ugly. Now when it's explicitly OK to not fail for invalid values (not an actual existing address) but merely doesn't resolve to an address, I don't see any reason to not try every value that could remotely resemble a valid address.

The thing is, the values we do try to use are from a very specific narrow part of the whole Dockerfile, namely default build arg values that is also used in the FROM instruction. So, any build arg that isn't used in the FROM instruction, won't be considered. Any build arg that doesn't have a default value.. same. etc. So, given that I don't think this is going to hit performance noticeably, and we shouldn't get any false hits either. (I could make a case up where this is not true, but I don't think we'll see that in practice.)

So I'm leaning towards option 1. How does that sound?

[huonw]

The thing is, the values we do try to use are from a very specific narrow part of the whole Dockerfile, namely default build arg values that is also used in the FROM instruction. So, any build arg that isn't used in the FROM instruction, won't be considered.

Ah okay, cool. To confirm, does it also apply to FROM //some/path/to:target?

[kaos]

The thing is, the values we do try to use are from a very specific narrow part of the whole Dockerfile, namely default build arg values that is also used in the FROM instruction. So, any build arg that isn't used in the FROM instruction, won't be considered.

Ah okay, cool. To confirm, does it also apply to FROM //some/path/to:target?

No, it does not, as Pants doesn't patch/edit the Dockerfile, so there would not be a way to replace that value with the actual image name.

[huonw]

Looks good.

[huonw]

The thing is, the values we do try to use are from a very specific narrow part of the whole Dockerfile, namely default build arg values that is also used in the FROM instruction. So, any build arg that isn't used in the FROM instruction, won't be considered.

Ah okay, cool. To confirm, does it also apply to FROM //some/path/to:target?

[WorkerPants]

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.19.x

Successfully opened #20658.

✔️ 2.20.x

Successfully opened #20657.

---

Thanks again for your contributions!

🤖 Beep Boop here's my run link