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

Error out on any changes to files in the repository after generation #159

Merged
merged 2 commits into from
Jan 31, 2023

Conversation

pradyunsg
Copy link
Member

This ensures that untracked files are correctly accounted for.

@pradyunsg
Copy link
Member Author

x-ref #158, which should be rebased once this is merged.

@pfmoore
Copy link
Member

pfmoore commented Sep 3, 2022

To confirm, the point of this check is to ensure that the generate script doesn't (inadvertently or maliciously) add extra files to the public tree, as those will be published on bootstrap.pypa.io. Is that correct?

If that's true, then what's the intended workflow for a change that modifies generate.py? Should that change include the newly generated files? Presumably that means that they would be tracked as part of the PR, so CI would work as expected? Of course, that means that any change to the generation process has to publish the newly generated (or changed) files immediately, and cannot wait until a new pip release. Is that what we want?

If we want this level of validation, while still allowing for the possibility of merging changes to the generation before publishing the changed files, we probably need to make generate.py build in a staging area and only have the release process modify public (by copying the staging area). Then we can either hard-code the files we want to be published into the release script (so any new files will show up in a change to that script) or we simply rely on the fact that the release manager should be manually checking that the release is sound before pushing the "publish" button. My preferred option is the latter.

@pradyunsg
Copy link
Member Author

pradyunsg commented Sep 4, 2022

If that's true, then what's the intended workflow for a change that modifies generate.py? Should that change include the newly generated files?

Yes.

Of course, that means that any change to the generation process has to publish the newly generated (or changed) files immediately, and cannot wait until a new pip release. Is that what we want?

We can make the generation conditional on the current pip version, like only generating the zipapp for pip>=22.3.

That would mean landing the PR with the new generation code deactivated at the time of writing. I’d actually prefer to publish it for the current version of pip and test it, while not advertising it and having a cautionary comment in there. It’s easier than having a staging environment and achieves most of the goals we have anyway.

@pfmoore
Copy link
Member

pfmoore commented Sep 4, 2022

I’d actually prefer to publish it for the current version of pip and test it, while not advertising it and having a cautionary comment in there.

Cool, I'm completely on board with that. So this PR needs pip.pyz and pip-22.2.2.pyz adding to it. What do you mean by "a cautionary comment" though? I can add a note to the readme in this repo, but it would need removing when we release 22.3.

I still need to put together some docs for pip announcing the new .pyz, so until those are in place (which will be part of 22.3) the new .pyz will be undocumented, and hence unsupported by default.

@pradyunsg
Copy link
Member Author

Disregard the cautionary comment bit, I was thinking of the pyz as a regular text/Python file instead; which it obviously isn’t. :)

This ensures that the generated files match exactly what `generate.py`
would generate.
These are difficult to validate re-generation of since the current
generation logic isn't versioned.
@pradyunsg
Copy link
Member Author

pradyunsg commented Jan 30, 2023

Alrighty, what should we do with the versioned .pyz files @pfmoore?

I've tentatively removed the versioned .pyz files with the intention of landing this with them removex (i.e. removing them from bootstrap.pypa.io). We can later add them in a separate follow up (or pivot them to be providing on a per-Python version specific manner). Would that be OK with you?

@pfmoore
Copy link
Member

pfmoore commented Jan 31, 2023

I don’t know if people rely on the versioned pyz files. As long as the unversioned one is there for now, I think that’s ok. But I do think we should work out a solution so we can publish versioned ones (pip version, not python version). I’m afraid I still don’t understand the reasoning behind the way this check works, so I may be being naive, but in my view if the check is giving us issues publishing versioned pyz files, it’s the check that should change, not what we publish.

In practical terms, though, I concede that we have no evidence yet that real users care either way 🙂

@pradyunsg pradyunsg merged commit 90813d6 into pypa:main Jan 31, 2023
@pradyunsg pradyunsg deleted the error-on-all-file-changes branch January 31, 2023 01:35
@pradyunsg
Copy link
Member Author

We might also want to add per-version get-pip.py files at that point, which... isn't the worst idea. :)

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