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

Ensure lockfile target exists before injecting a dependency to it. #17365

Merged

Conversation

kaos
Copy link
Member

@kaos kaos commented Oct 27, 2022

Fixes #17343

Example warning message when the _lockfiles target is being shadowed:

09:25:43.12 [WARN] The synthetic lockfile target for 3rdparty/python/user_reqs.lock is being shadowed by the python_requirements target 3rdparty/python:python-default.

There will not be any dependency to the lockfile.

Resolve by either renaming the shadowing target, the resolve 'python-default' or moving the target or the lockfile to another directory.

@kaos kaos added the category:bugfix Bug fixes for released features label Oct 27, 2022
@asherf
Copy link
Member

asherf commented Oct 31, 2022

@Eric-Arellano @stuhood ping

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, does this mean that if you have your repo in the wrong shape that we simply don't add the dependency at all on lockfile targets? That also seems broken.

What about your earlier idea to instead use an unambigous target name? If that's too hard to performantely check which target names already exist, then use some special value extremely unlikely to be claimed, like __resolve_my_resolve__

@kaos
Copy link
Member Author

kaos commented Oct 31, 2022

What about your earlier idea to instead use an unambigous target name?

The issue was that we don't know the other targets at the time we need to pick our target name. Going with a very obtruse name to avoid a potential name collision feels.. not great. This fix avoids a real issue at the cost of not getting the benefit of this new feature, with the easy fix of adjusting either the conflicting target's name or the name of the resolve, or moving either the lockfile or the conflicting target to another path. (just to give a hint that there are more than one way to work around the issue presented in #17343)

@Eric-Arellano
Copy link
Contributor

Maybe we log then a warning about the collision? And add this edge case to the help message?

On that note, would be good to use a rule_helper for DRY

@kaos kaos added this to the 2.15.x milestone Nov 3, 2022
@kaos
Copy link
Member Author

kaos commented Nov 4, 2022

@Eric-Arellano @stuhood ping. I think we may want to pick this to 2.15.x as well.

@kaos
Copy link
Member Author

kaos commented Nov 8, 2022

@stuhood I'd like to rebase #17451 on top of this one to avoid merge conflicts for the synthetic target tests (that is, before I start writing more tests for synth targets for that PR there's changes in here I'd like to make use of)

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

This problem seems loosely related to the idea of "hygienic macros": i.e., that a macro should be able to define anonymous variables which can't collide with or pollute the original namespace. But I don't know how to do that and still have consumers later depend-on/find them... maybe by having the request for the syntethic target use something other than its name/address?

In any case: shipping to unblock the fix.

@kaos kaos merged commit 0189342 into pantsbuild:main Nov 8, 2022
@kaos kaos deleted the issue/17343-synthetic-target-name-collision branch November 8, 2022 23:51
@kaos kaos mentioned this pull request Nov 8, 2022
kaos added a commit that referenced this pull request Nov 9, 2022
…17365)

* Ensure lockfile target exists before injecting a dependency to it.

* add feature toggle for python lockfiles targets.

* Refactor requirement generators to use a common rule helper.

* Warn when lockfile targets are being shadowed.

* make ignored arg optional
kaos added a commit that referenced this pull request Nov 9, 2022
…herry-pick of #17365) (#17505)

Ensure lockfile target exists before injecting a dependency to it. (#17365)

* Ensure lockfile target exists before injecting a dependency to it.

* add feature toggle for python lockfiles targets.

* Refactor requirement generators to use a common rule helper.

* Warn when lockfile targets are being shadowed.

* make ignored arg optional
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synthetic targets for lockfiles break when a target has the same name as the resolve
4 participants