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 release task build dists @ clean tmp checkout #7654

Merged
merged 15 commits into from
Feb 10, 2020

Conversation

webknjaz
Copy link
Member

This is a follow-up of (and includes the commits from) #7631.

I did some hacking and submitting some PoC for the initial feedback.

The idea here is to checkout the given git ref under /tmp, build there and copy only the final dists back to the normal workdir.

@webknjaz webknjaz requested review from pradyunsg and pfmoore and removed request for pradyunsg January 26, 2020 02:11
noxfile.py Show resolved Hide resolved
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

The overall flow looks about right to me, and the worktree checkout seems right to me? I didn't actually drive this fully, because, the sane-release-state checks are correctly blocking that and I need to eat lunch. :)

Some comments around code organization (since we have a tools/automation/release/__init__.py file for putting helpers in), using of nox-provided helpers and some logging-related (soft) suggestions.

There's also a few minor nit-picks that definitely be ignored if you disagree, but it'd be nice if you'd incorporate them if you're ambivalent about them. :)

noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated
yield dir_path
finally:
nox_session.log(f"# Changing dir back to {orig_dir}")
os.chdir(orig_dir)
Copy link
Member

Choose a reason for hiding this comment

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

nox's Session.chdir() should be used here, to replace both this and the line above this: https://nox.readthedocs.io/en/stable/config.html#nox.sessions.Session.chdir

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it doesn't have any args for extra message :(
I wanted to emphasize that this is changing back.

noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated
Comment on lines 216 to 221
session.log("# Checkout the master branch")
session.run("git", "checkout", "master", external=True, silent=True)

Copy link
Member

Choose a reason for hiding this comment

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

Yay! ^>^

noxfile.py Show resolved Hide resolved
@webknjaz webknjaz force-pushed the misc/tmpdir-nox-build branch 6 times, most recently from 88e495f to d4bf0fa Compare January 28, 2020 16:39
@webknjaz
Copy link
Member Author

Failing CI seems unrelated...

@pradyunsg
Copy link
Member

Rebasing on master should fix the CI. :)

noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@webknjaz webknjaz force-pushed the misc/tmpdir-nox-build branch from d4bf0fa to e23f44c Compare February 3, 2020 09:45
@webknjaz webknjaz force-pushed the misc/tmpdir-nox-build branch from 07df75d to 5b05416 Compare February 3, 2020 09:54
@webknjaz webknjaz force-pushed the misc/tmpdir-nox-build branch from 5b05416 to 6f1a43e Compare February 3, 2020 10:00
@webknjaz webknjaz force-pushed the misc/tmpdir-nox-build branch from a94c450 to 9d592b3 Compare February 3, 2020 10:14
@webknjaz webknjaz requested a review from pradyunsg February 3, 2020 11:54
@pradyunsg pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Feb 6, 2020
@pradyunsg pradyunsg merged commit c24aba5 into pypa:master Feb 10, 2020
@pradyunsg
Copy link
Member

Merged since this LGTM and, well, it's lingered for quite a while anyway.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Mar 11, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants