-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CI: use pipx and build to simplify and remove a workaround #43157
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
Conversation
@@ -38,17 +38,9 @@ jobs: | |||
with: | |||
python-version: ${{ matrix.python-version }} | |||
|
|||
- name: Install dependencies | |||
run: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea is to simulate core building and distributing this application. how does this improve the experience here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Artifact building should happen with build
, as well. build
will respect PEP 518 requirements, raw setup.py
commands cannot, which is why the workaround in #39416 was introduced. Making an SDist should be done with pip install build && python -m build --dist
(or pipx run build --sdist
), not pip install setuptools wheel numpy && python setup.py sdist --formats=gztar
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you replace with pip install build && python -m build --dist
prefer to use the standard tools here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pipx is a standard tool, it's part of the PyPA, just like build, setuptools, and pip. I can change this if preferred, but then you are dependent on setup-python@v2
, while pipx is not.
this doesn't help as the archives are not generated here (they are actually justed tested here). sure i suppose we could move to this workflow, but that should happen in conjuction with a change like this. |
Where are the SDist's built? https://github.com/MacPython/pandas-wheels only does wheels, AFAICT. Also, this is still testing that the artifact builds and is installable, it's just using better infrastructure for doing so without manual workarounds in producing them. |
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
- name: Build pandas sdist | ||
run: | | ||
pip list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we usually want this line so that we can see what versions are installed when debugging CI failures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the build environment. No version listed here would have anything to do with what you have inside the PEP 517 build environment. If you use pipx, as below, then even the version of build is not listed and the version of Python is unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've dropped setup-python, which should reduce confusion here, and added a --version
run so that the logs will have the build version.
80ee068
to
29f45b7
Compare
@simonjayhawkins do we have any manual process in the release for sdists? i don't recall if this changed recently. |
note completely averse here but this would need to address comments & rebase |
Thanks for the PR, but it appears to have gone stale. If interested in continuing please merge the main branch, address the review, and we can reopen. |
I've rebased, but you can't reopen a PR after force pushing to it (IIRC). I think this was stuck because there was some "manual/other release process" that also needed updating but I wasn't pointed to that. |
Ah okay. Yes I see we can't reopen this PR (we also don't recommend force pushing to branch as it makes the commit history and changes harder to view). If you'd like to continue with a new PR we can try to contact the right person who has set up sdist. |
There weren't really any changes yet, it was just tracking the current main. I can retry. |
yes, the sdist is created locally during the release process and then uploaded to the github release artifacts so that conda-forge can use it and also uploaded to PyPI
https://github.com/pandas-dev/pandas-release and the conda recipe that points to the manually created sdist uploaded as a github release artifact is at |
This cleans up the SDist build slightly. First, it uses PyPA's "build" instead of directly calling
setup.py
, which will respect PEP 518 requirements and avoids the workaround of installing NumPy to be able to run setup.py. It also uses pipx to avoid needless setup; PyPA's pipx is provides as a first-class package manager on GitHub Actions (and Azure, they share images).