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

Make lock update sensitive to artifacts. #1887

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Aug 23, 2022

Previously lock updates were sensitive to changes in pins but not to
changes in the set of artifacts when pins remained unchanged.

Previously lock updates were sensistive to changes in pins but not to
changes in the set of artifacts when pins remained unchanged.
Previously there was ambiguity in the key used to store built wheels when the
local OS (platform tag) could take on different values as is the case in
macOS Big Sur where 10.16 and 11.0 are synonyms.
@jsirois
Copy link
Member Author

jsirois commented Aug 23, 2022

The second commit is needed for green CI but is off for seperate review in #1886.

@jsirois
Copy link
Member Author

jsirois commented Aug 23, 2022

This shortcoming in functionality was reported by @zmanji in Slack - no associated issue.

class Value(Enum.Value):
pass

IGNORE = Value("ignore")
Copy link

Choose a reason for hiding this comment

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

Does ignoring in this case amount to adjusting the lockfile to use a new fingerprint? Or will it keep the old fingerprint? Should this have a different name that clarifies that?

Copy link
Member Author

Choose a reason for hiding this comment

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

When you ignore the mismatch, the new fingerprint is used. I expanded the corresponding option help to document what each value does.

def _calculate_requirement_configuration(
self,
locked_resolve, # type: LockedResolve
pin_all=False, # type: bool
Copy link

Choose a reason for hiding this comment

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

Given that this is pin_all (implying that a different scope of pinning might be necessary/useful), should the --pin flag maybe start as either an enum, or perhaps a list option with an initial special-case value of e.g. :all:?

Copy link
Member Author

@jsirois jsirois Aug 24, 2022

Choose a reason for hiding this comment

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

I don't think so. The two existing modes of lock update are:

  1. no args: Update the lock given its original input root requirements (these are stored in a lockfile)
  2. -p one -p "two<2" -p three==1.0 ...: Update the lock pinning everything else and just updating (or adding) these projects if possible.

So this just offers the only missing mode as far as I can tell (besides allowing for project removal) - update the lock keeping everything --pinned.

Functionally, the 1st mode updates the lock to the latest available needed to satisfy the root requirements. The second mode is a targeted update - I need a specific security fix or bug fix. This third mode allows just switching indexes / repos or picking up newly published artifacts. You can do this with the 1st two modes, but they also force you to change some pin. In order to not change a pin, you'd have to use mode 2 and pick one project pin from the existing lock by inspecting it and then use that on the CLI to effect the new --pin (all) behavior; so --pin is just a short cut for a human and a firewall for a machine - which should not be trying to parse an opaque-still lock file format it has no business knowing.

For all other updates you simply must use create instead - at that point it becomes a new lock.

@jsirois jsirois merged commit d154355 into pex-tool:main Aug 24, 2022
@jsirois jsirois deleted the lock/update/repos-switch branch August 24, 2022 14:04
@jsirois jsirois mentioned this pull request Aug 24, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants