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

feat: lock file format 2.0 (was: store package files on the package in lockfile) #6393

Merged

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Sep 4, 2022

counter-proposal to #6389, per #6389 (comment)

doesn't quite fix #6327, we continue to write the unwanted extra metadata.files line to the lockfile until some future point where we stop writing that section at all does fix #6327 after all now I think harder about it, the unwanted files and hashes don't get wrongly attached to the URL package after this

also fixes #6349, installing a URL package doesn't care any more about files and hashes from other packages.

@dimbleby dimbleby force-pushed the package-files-belong-on-package branch 3 times, most recently from 51ab32a to 6dc4c51 Compare September 5, 2022 07:47
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

We should add a test poetry can still read the old format. And while we're on it, we can add a test for the even older "only hash" format, too.

@@ -241,6 +250,7 @@ def set_lock_data(self, root: Package, packages: list[Package]) -> bool:
"lock-version": self._VERSION,
"python-versions": root.python_versions,
"content-hash": self._content_hash,
# TODO stop writing files here, this is deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

If we bump the lock-version, there is no reason to keep writing files here, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends whether we're ready to make a breaking change or not.

If we stop writing files here then the version should go 2.0, so that down-level poetry knows that it can't read it.

I was expecting that we'd go 1.2 now, and 2.0 at some later point, but am happy to be told otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I thought that any version bump makes it unreadable for older poetry. (I didn't look into the code.)

I have concerns that the lockfile size will increase significantly if we write the information twice. It seems we can only choose between the devil and the deep blue sea... I think we have to discuss that among maintainers.

@dimbleby
Copy link
Contributor Author

dimbleby commented Sep 9, 2022

We should add a test poetry can still read the old format. And while we're on it, we can add a test for the even older "only hash" format, too.

There are already tests using the old_lock fixture.

Given that even that doesn't use the even-older format I think it might actually be about time to remove the code that reads it. Looks like it was removed at b1c4c68, three years ago, it's probably reasonable for the next poetry release to finally drop support for that - there must be approximately zero examples left in the wild.

@dimbleby
Copy link
Contributor Author

dimbleby commented Sep 9, 2022

it turns out poetry's own unit tests use the old lockfile format, I've updated them to the 2019 vintage in this MR and also in python-poetry/poetry-plugin-export#119

Updating them to use the format proposed here is more tedious, I'm inclined to say that we can live without doing that. Anyway it might be some time before I can work up enthusiasm for the task...

@dimbleby dimbleby force-pushed the package-files-belong-on-package branch from fa06230 to 808ecbc Compare September 9, 2022 23:50
@neersighted neersighted added area/solver Related to the dependency resolver kind/refactor Pulls that refactor, or clean-up code status/needs-consensus Consensus among maintainers required labels Sep 17, 2022
@neersighted neersighted added this to the 1.3 milestone Sep 17, 2022
@dimbleby dimbleby force-pushed the package-files-belong-on-package branch from 808ecbc to 21248f7 Compare September 19, 2022 22:40
@dimbleby
Copy link
Contributor Author

it occurs to me that another thing that could be done to soften the blow of changing the file format would be to backport the reading (but not the writing) part of the change to 1.2.

Maybe that would make it more palatable to do the breaking change all in one go, with no intermediate write-everything-in-two-places

  • poetry 1.2.0 and 1.2.1 would still see a breaking change and refuse to use a poetry 1.3-written lockfile
  • but poetry 1.2.2 (say) could use that lockfile just fine

being forced to 1.2.n is likely more tolerable to some than being forced to 1.3. And if that 1.2.n were released some moderate period ahead of 1.3.0, then some of those who would have been annoyed by a breaking change in format instead won't even notice it.

@radoering
Copy link
Member

I like the idea. 👍

@dimbleby
Copy link
Contributor Author

pushed a commit to go straight to 2.0 in this MR, and remove the old metadata.files completely

#6608 is the proposed backport to 1.2.

@neersighted neersighted changed the title store package files on the package in lockfile feat: lock file format 2.0 (was: store package files on the package in lockfile) Sep 23, 2022
@radoering radoering removed the status/needs-consensus Consensus among maintainers required label Sep 24, 2022
radoering pushed a commit that referenced this pull request Sep 24, 2022
Backport the ability to read 2.0 lockfiles per #6393
@radoering radoering force-pushed the package-files-belong-on-package branch from 76f1850 to e7f7f46 Compare September 24, 2022 11:55
@radoering radoering enabled auto-merge (squash) September 24, 2022 11:57
@radoering radoering merged commit c061ac5 into python-poetry:master Sep 24, 2022
@dimbleby dimbleby deleted the package-files-belong-on-package branch September 24, 2022 13:04
@neersighted neersighted removed the area/solver Related to the dependency resolver label Sep 24, 2022
@neersighted neersighted added the area/pyproject Metadata/pyproject.toml-related label Sep 24, 2022
yurnov added a commit to yurnov/electricitybot that referenced this pull request Dec 14, 2022
  the reason for this change is to keep the old lock file format and
  avoid incompatibility with version in main of F1ashhimself/electricitybot

  later on version can be changed to 1.3.1 or any other later version
  (requrie changes in .github/workflows/app.yaml, Dockerfile and update
  version on local enviroment)

  python-poetry/poetry#6393
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/pyproject Metadata/pyproject.toml-related kind/refactor Pulls that refactor, or clean-up code
Projects
None yet
3 participants