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

refactor: generate requirements files #1055

Merged
merged 1 commit into from
May 24, 2021

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Mar 30, 2021

Closes #1019, based on the similar structure in cibuildwheel. Suggested by @mayut to improve maintainability.

To run locally, either install all versions of Python or use docker, two examples below:

docker run --rm -v $PWD:/nox -t thekevjames/nox nox -f /nox/noxfile.py -s compile tools
docker run --rm -v $PWD:/nox -t quay.io/pypa/manylinux2010_x86_64:latest pipx run nox -f /nox/noxfile.py -s compile tools

@henryiii henryiii force-pushed the refactor/requirements branch 3 times, most recently from dead6f1 to 617e4fd Compare March 30, 2021 01:36
Copy link
Member

@mayeut mayeut left a comment

Choose a reason for hiding this comment

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

Thanks @henryiii, this looks good overall.
One minor issue is that we'll lose what's been bumped in the PR description and the change logs from dependabot but those minor details are really not showstoppers and, at least for what's been bumped, one can look at the commit itself rather than its description.

docker/build_scripts/finalize.sh Outdated Show resolved Hide resolved
@mayeut
Copy link
Member

mayeut commented Apr 3, 2021

I might use this PR as a baseline to continue with the automated update thingy I worked on a couple months ago.
I also wanted to have a look at nox (though not for manylinux) so this might be the occasion.

@henryiii henryiii marked this pull request as ready for review April 12, 2021 03:58
@henryiii
Copy link
Contributor Author

@mayeut Should I add docs? If you think this approach is better, let me know how you want it finished off.

@mayeut
Copy link
Member

mayeut commented May 2, 2021

@henryiii,

I'll have another look once CPython 3.5 is dropped (and CPython 3.10 added)

@mayeut
Copy link
Member

mayeut commented May 14, 2021

Now that #1090 is merged in, I wonder if nox or pipx would be good additions in order to run nox directly in the manylinux image.

See also:
pypa/cibuildwheel#661

@henryiii
Copy link
Contributor Author

I used #1090 to run nox in manylinux to get Python 3.10. :)

@mayeut mayeut mentioned this pull request May 14, 2021
@henryiii
Copy link
Contributor Author

Note: if the nox action on GHA doesn't include pre-release Pythons, then we'll need one extra setup-python with 3.10.

@henryiii
Copy link
Contributor Author

PS: It's also still very much up to you which method you'd rather maintain. :)

@mayeut
Copy link
Member

mayeut commented May 14, 2021

Note: if the nox action on GHA doesn't include pre-release Pythons, then we'll need one extra setup-python with 3.10.

Now that we talked about that both here & over at cibuildwheel, I opened #1095 with the addition of pipx and it would now makes sense, I think, to use one of the manylinux image to run the job (& in the doc).

For the CI job, I'd go with manylinux_2_24_x86_64 which is the smallest one.

For the docs, it might be worth mentioning that any manylinux2010+ image can be used.

Edit:
And it seems the nox image is missing CPython 3.10 for now:

Matt$ docker images
REPOSITORY                             TAG                  IMAGE ID       CREATED         SIZE
quay.io/pypa/manylinux_2_24_x86_64     latest               89dbeffaa06f   2 hours ago     686MB
thekevjames/nox                        latest               218ede80c86e   22 hours ago    715MB

Matt$ docker run --rm -it thekevjames/nox:latest
root@0145d42645eb:/# python
python-argcomplete-check-easy-install-script  python3.5          python3.6m         python3.8
python-argcomplete-tcsh                       python3.5-config   python3.6m-config  python3.8-config
python2.7                                     python3.5m         python3.7          python3.9
python2.7-config                              python3.5m-config  python3.7-config   python3.9-config
python3                                       python3.6          python3.7m
python3-config                                python3.6-config   python3.7m-config                             

@henryiii
Copy link
Contributor Author

henryiii commented May 14, 2021

I don't think we want to run the CI job in the image, since it's probably faster to use the native actions over pulling a docker container (the Pythons are pre-installed), and you get into a chicken-and-egg problem if you add a new Python version.

(Yes, 3.10 is missing in the docker image, which is why I used #1090 to run the 3.10 job in manylinux, manually adding nox - should be much simpler now)

@mayeut
Copy link
Member

mayeut commented May 14, 2021

I don't think we want to run the job in the image, since it's probably faster to use the native actions over pulling a docker container, and you get into a chicken-and-egg problem if you add a new Python version.

Sorry, I missed the fact that the nox image wasn't used in the job. I'm understanding your previous Note correctly now.
I think still my comment is valid for the docs though.

For the chicken-and-egg problem, If we were to use a manylinux image, my best guess is that when adding a new python version, we'd just need to copy/paste the previous python version requirement file to bootstrap the whole process. The job would then just pick that up no ? It doesn't happen that often anyway for it to be really troublesome

README.rst Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@henryiii henryiii force-pushed the refactor/requirements branch 3 times, most recently from 79c298f to 82531ff Compare May 15, 2021 00:13
@henryiii
Copy link
Contributor Author

No more locale bug for < 3.7 with the manylinux image. :)

@henryiii henryiii force-pushed the refactor/requirements branch 4 times, most recently from b308c0e to db0c47b Compare May 15, 2021 01:51
@henryiii
Copy link
Contributor Author

Checked it with the update workflow turned on on an earlier build, looks like it's working fine. There might be some changes needed due to the token requirement - see @joerick's workaround on cibuildwheel with a GitHub app token.

@mayeut
Copy link
Member

mayeut commented May 15, 2021

There might be some changes needed due to the token requirement - see @joerick's workaround on cibuildwheel with a GitHub app token.

I've been meaning to ask pypa about a bot / app when saw his work but forgot since then...
That's the last thing to do because otherwise, everything looks good.

@henryiii
Copy link
Contributor Author

henryiii commented May 17, 2021

pypy/manylinux will need minor changes, I'm looking at https://github.com/pypy/manylinux/blob/8650a6d4a534e72dd3f5d1b38327cc94e404986b/docker/build_scripts_pypy/install_pypy.sh#L46 specifically, which will need the version number added, requirements.txt -> requirements37.txt and such. Guessing @asottile or @mattip should be aware of the change coming in? I'd be up for making a PR once this goes in if needed.

Wouldn't it be possible to add pypy to the pythons in pypa/manylinux? I think this was hinted at in pypy/manylinux#7, and would solve the issue in pypy/manylinux#15.

@mattip
Copy link
Contributor

mattip commented May 17, 2021

Wouldn't it be possible to add pypy to the pythons

Maybe open a separate issue for this?

@henryiii henryiii mentioned this pull request May 17, 2021
@henryiii
Copy link
Contributor Author

henryiii commented May 17, 2021

Actually, it looks like pypy/manylinux is based on an old version of pypa/manylinux; notably it has a copy of requirements.txt internally (which is missing other important things we've added in this repo, mostly build).

@mayeut mayeut self-requested a review May 24, 2021 09:42
Copy link
Member

@mayeut mayeut left a comment

Choose a reason for hiding this comment

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

Let's get this in.
I'll deal with the bot a bit later.

@mayeut mayeut merged commit 95d3a09 into pypa:master May 24, 2021
mayeut added a commit to mayeut/manylinux that referenced this pull request May 24, 2021
mayeut added a commit that referenced this pull request May 24, 2021
backport #1055

Co-authored-by: Matthieu Darbois <[email protected]>

Co-authored-by: Henry Schreiner <[email protected]>
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.

pip-compile based requirements.txt
3 participants