Update address parameters on overrides

[isra17]

This fix a bug where default parametrize resolve could not get overriden.

The fix ensure that an address parameters are updated on any override, not just parametrized one.

Fixes #20933

[benjyw]

Thanks for the fix @isra17 ! To get CI passing, add an entry in docs/notes/2.21.x.md (since this is a bugfix we'll cherrypick it to 2.21.0 before releasing that), and also fix whatever the lint shard is complaining about (pants --changed-since=main fmt fix check lint should do the trick).

[huonw]

Thanks for the fix!

It looks like this reproduces in at least 2.17.0 (thanks for the reproducer in #20933), which suggests to me:

• let's cherry pick back to 2.19
• we shouldn't try to get this into 2.21.0 stable:
  • as release manager, I'm feeling like we're pretty close to being good to release 2.21.0, with minimal bugs reported about it, specifically (and 2.21 release preparation #20846 suggest 6 weeks since 2.20.0 would be May 28, next week)
  • any change risks delaying the release of all the new features (e.g. if that change has a minor mistake), and so preferably we only do necessary/urgent ones
  • this doesn't feel like a urgent bug fix, because it's not a regression since 2.20: if we release 2.21.0 with it not-fixed, no existing code is going to be newly affected (it's already affected in 2.20 and earlier!).

Thus, I'd suggest we put the notes in docs/notes/2.22.x.md since 2.22.0 will be the first .0 release that contains it.

[kaos]

Thanks for the find, repro! and fix 🙌🏽

I think we'd like to take it the full mile however, and remove any "unparametrized" parameters from the address as well.

[isra17]

Ok, so this PR grew a bit in scope trying to attempt #20934 (comment) .

In short, I've added support to:

• Unparametrize address when expanding them with single values: Address("a@f=x").expand({"f": "y"}) => Address(a).
• Manage single value Parametrize the same as a single value (Handle f="x" the same as f=Parametrize("x")).

I also went a bit farther and unparametrize from group as well, so that if a parameters group has resolve="other-resolve", then it's going to remove the resolve parameter from the address. This might fix other similar bug than the original one.

Let me know what you think and if you would rather have me revert to the original scope.

[kaos]

Ok, so this PR grew a bit in scope trying to attempt #20934 (comment) .

In short, I've added support to:

• Unparametrize address when expanding them with single values: Address("a@f=x").expand({"f": "y"}) => Address(a).
• Manage single value Parametrize the same as a single value (Handle f="x" the same as f=Parametrize("x")).

I also went a bit farther and unparametrize from group as well, so that if a parameters group has resolve="other-resolve", then it's going to remove the resolve parameter from the address. This might fix other similar bug than the original one.

Let me know what you think and if you would rather have me revert to the original scope.

Love this!

I think we may want to go back to not unparametrize single value parametrizations, but lets hold on that until we get more opinions.

[kaos]

yea, looking at this code I feel it's kind of hairy.

Perhaps split this to fix the original issue in this PR, and have a new PR addressing the more general case?
wdyt?

[isra17]

Ok, third round!

• I removed the new logic that normalize single value parametrize into non-parametrize (I can open another PR with these change if there's any interest for this behavior).
• I reimplemented the change with the suggestion of tracking a parameters dict and creating a new address once.

I think CI should get green this time 🤔

[kaos]

This looks great, thanks!

Just some minor nits and tweaks left, I think :)

[isra17]

Comment addressed and CI is green!

[kaos]

Great.

(n.b. would prefer regular pushes over force push when possible, makes it easier to review what's changed since last review. we do squash merges so it doesn't mess up the history either way.)

[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

I was unable to cherry-pick this PR to 2.19.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

1. (Ensure your git working directory is clean)
2. Run the following script to reproduce the merge-conflicts:

   git fetch https://github.com/pantsbuild/pants main \
     && git fetch https://github.com/pantsbuild/pants 2.19.x \
     && git checkout -b cherry-pick-20934-to-2.19.x FETCH_HEAD \
     && git cherry-pick 408d29182de5ea9f170e253f303412ed56407181

3. Fix the merge conflicts and commit the changes
4. Run build-support/cherry_pick/make_pr.sh "20934" "2.19.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

❌ 2.20.x

I was unable to cherry-pick this PR to 2.20.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

1. (Ensure your git working directory is clean)
2. Run the following script to reproduce the merge-conflicts:

   git fetch https://github.com/pantsbuild/pants main \
     && git fetch https://github.com/pantsbuild/pants 2.20.x \
     && git checkout -b cherry-pick-20934-to-2.20.x FETCH_HEAD \
     && git cherry-pick 408d29182de5ea9f170e253f303412ed56407181

3. Fix the merge conflicts and commit the changes
4. Run build-support/cherry_pick/make_pr.sh "20934" "2.20.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

❌ 2.21.x

I was unable to cherry-pick this PR to 2.21.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

1. (Ensure your git working directory is clean)
2. Run the following script to reproduce the merge-conflicts:

   git fetch https://github.com/pantsbuild/pants main \
     && git fetch https://github.com/pantsbuild/pants 2.21.x \
     && git checkout -b cherry-pick-20934-to-2.21.x FETCH_HEAD \
     && git cherry-pick 408d29182de5ea9f170e253f303412ed56407181

3. Fix the merge conflicts and commit the changes
4. Run build-support/cherry_pick/make_pr.sh "20934" "2.21.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

---

When you're done manually cherry-picking, please remove the needs-cherrypick label on this PR.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

[isra17]

Great.

(n.b. would prefer regular pushes over force push when possible, makes it easier to review what's changed since last review. we do squash merges so it doesn't mess up the history either way.)

Thank you for your patience!

In the future I will keep commits stack if that make your job easier ;)

[kaos]

@huonw I don't have the bandwidth currently to look into the failed cherry picks. Could be that the merge conflicts are due to changes in the parametrize.py that hasn't been picked.

Not sure how urgent it is to actually get this fix picked?

@isra17 are you tied down to a particular Pants version, and would prefer this fix on that release branch, or are you able to upgrade to an upcoming 2.22 dev/rc?

[huonw]

If there's changes that haven't been picked, that's a possibility.

Another likely candidate is the release notes addition: the 2.22 file won't exist in older versions. This is a "known issue" unfortunately, e.g. #20888 (comment)

[kaos]

Ah, that could be it as well then. I thought I had some relevant changes, but I think those've been picked.

[isra17]

No worry on our side, we did went with a workaround so it's not a blocker (And we also don't mind to upgrade to dev/rc)