🔥 Explode on potentially rubbish files in the workdir that aren't Git-tracked

[webknjaz]

This is a follow-up improvement for
#7624

[pfmoore]

With this addition, presumably this script is only safe to run on a clean checkout? Is there any way to check and make sure no-one mistakenly runs this in their development checkout?

Rather than deleting unchecked files, could the script not abort if it finds any, and get the user to clean up manually?

Edit: The advantage of the existing code that only clears the build directory is that the build directory is by design intended to be temporary, and safely removable.

[webknjaz]

@pfmoore well, I suppose we could do that with --dry-run.

P.S. Ideally the script should do git worktree into some tmp location and do all the stuff there which combined with git clean would make things even cleaner.

[pfmoore]

See my comment, which should have been submitted as a review instead 🙁

[webknjaz]

(already answered: #7631 (comment))

[pfmoore]

Apart from the build directory, do untracked files actually pose any risk?

[webknjaz]

@pfmoore yes, sometimes you can accidentally include .pyc or some garbage. Even if you have an explicit blacklist, this doesn't protect from future changes: the human factor can kick-in any moment and unwanted things would appear somewhere else. There can be some other package/module name collision with a similar outcome...

[ewjoachim]

I haven’t looked closely enough whether in the local environment, pip itself is installed in -e mode, but it is sufficiently standard in environments so I’ll go with it, please excuse me if irrelevant:

What about the pip.egg-info folder ? You may not want to delete that, I’d guess (or you’ll have to reinstall pip locally afterwards)
What about the potential personal files, like workspace editor configuration, that might be there ? Whoever does the release might not appreciate loosing that.

[webknjaz]

@ewjoachim this is about the clean-up before building. So it's good to remove artifacts of previous builds. Also, I'm pretty sure that building dists is not related to editable installs.

[pfmoore]

Rather than trying to clean up the checkout that we're running the automation in, wouldn't it be better if the release automation checked out a completely clean new copy of pip master? That way there's no doubt we have a clean starting point...

One possible complication with such an approach would be if people want to build the release in their personal fork, and then make a PR for it. I'm not sure if that's something @pradyunsg does, for example. The docs don't discuss that possibility, though, so if it is an approach we want to allow, I'd rather write it up and then modify the tools to support it (for example, I'm not clear how I'd set the release tag correctly if I did the release via a PR).

[pradyunsg]

wouldn't it be better if the release automation checked out a completely clean new copy of pip master?

I think so yea, this would definitely be much cleaner IMO -- I've had to make changes to the working directory in the past (to the release process automation, or NEWS.rst file, for example) but I do agree that it'd help to have the actual checkout that the release is built from happen in not-the-same-worktree.

I think I much prefer that over the proposed approach, and very slightly over the current "clean the build directory before building things" approach.

---

Note that resetting all the files to what a "fresh" git checkout would look like, also removes the .nox directory which contains the python[.exe] we're running from -- that's what's causing the CI failures here.

[pfmoore]

Note that resetting all the files to what a "fresh" git checkout would look like, also removes the .nox directory

Ha. Good point. I'd been thinking of something like nox -s prepare_release -- 99.9 build_dir to build a new checkout in build_dir, then do the prepare in that. Leaving the current checkout unchanged, and never running nox from within the directory we're releasing from. But I hadn't thought through yet whether we'd want to add the same parameter to publish_release or whether we should run that from within the new checkout. As usual, it's the details that are tricky to get right.

Also, are you OK with releasing direct from master, or do you go via a PR? Personally, I feel safer doing a PR, but with our current docs I'd do it from master, as I'd rather trust the docs than guess how to do it any other way (my git skills aren't that good).

[pradyunsg]

Well, I'm supposed to be updating the docs, to reflect that we can't push to master (it's a protected branch). I allow admins to push temporarily when I'm not interested in the CI (eg. 20.0.2) to allow pushing to master and then re-enable the check within the same minute as I disable it.

I actually wanna automate the entire flow to a push-and-done flow, so that there's no need for such hacks, but baby steps.

[pfmoore]

OK cool (I'd forgotten master was protected). I can probably look at that. The thing I am least sure about was how to get the tags across, as they won't come as part of a PR.

But no rush, there's no more releases for 3 months so have a rest first :-)

[webknjaz]

as they won't come as part of a PR.

If you use a regular merge (with a proper merge commit), you could tag the commit in the PR branch and then it'd belong to master too after merging. Does this sound reasonable?

[webknjaz]

Folks, I've changed the PR to just check that the dir is clean. It should be safer now.

[pradyunsg]

Does this sound reasonable?

Yep -- that's how 20.0 was made. :P

[ewjoachim]

@ewjoachim this is about the clean-up before building. So it's good to remove artifacts of previous builds. Also, I'm pretty sure that building dists is not related to editable installs.

I do understand that. My point was that if we remove all untracked files, and not just those in the dist folder, we may delete untracked files that are not build artifacts at all (like those I listed).

That being said, 👍 on the new approach.

[webknjaz]

we may delete untracked files that are not build artifacts at all (like those I listed).

I think that the goal should be for those files to not exist when the build starts because it may cause problems over time when somebody changes something in the build system and they will get included...

[webknjaz]

@ewjoachim I've also sketched a more advanced solution here: #7654

[pradyunsg]

Closing this, in favor of #7654 (nice number!) -- I think the approach there is a bigger improvement over status quo than this PR is. :)