Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

henryiii
Copy link

  • closes #xxxx
  • tests added / passed
  • Ensure all linting tests pass, see here for how to run them
  • whatsnew entry

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).

@@ -38,17 +38,9 @@ jobs:
with:
python-version: ${{ matrix.python-version }}

- name: Install dependencies
run: |
Copy link
Contributor

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?

Copy link
Author

@henryiii henryiii Aug 21, 2021

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.

Copy link
Contributor

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

Copy link
Author

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.

@jreback
Copy link
Contributor

jreback commented Aug 21, 2021

PyPA's pipx is provides as a first-class package manager on GitHub Actions (and Azure, they share images).

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.

@henryiii
Copy link
Author

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.

@jreback jreback added Build Library building on various platforms CI Continuous Integration labels Sep 1, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2021

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.

@github-actions github-actions bot added the Stale label Oct 1, 2021
- name: Build pandas sdist
run: |
pip list
Copy link
Member

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

Copy link
Author

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.

Copy link
Author

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.

@henryiii henryiii force-pushed the henryiiii/ci/build branch from 80ee068 to 29f45b7 Compare October 5, 2021 20:36
@jreback
Copy link
Contributor

jreback commented Oct 6, 2021

@simonjayhawkins do we have any manual process in the release for sdists? i don't recall if this changed recently.

@jreback
Copy link
Contributor

jreback commented Jan 16, 2022

note completely averse here but this would need to address comments & rebase

@mroeschke
Copy link
Member

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.

@mroeschke mroeschke closed this Jan 25, 2022
@henryiii
Copy link
Author

henryiii commented Jan 25, 2022

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.

@mroeschke
Copy link
Member

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.

@henryiii
Copy link
Author

There weren't really any changes yet, it was just tracking the current main. I can retry.

@simonjayhawkins
Copy link
Member

@simonjayhawkins do we have any manual process in the release for sdists? i don't recall if this changed recently.

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

Where are the SDist's built? https://github.com/MacPython/pandas-wheels only does wheels, AFAICT.

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

https://github.com/conda-forge/pandas-feedstock

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Library building on various platforms CI Continuous Integration Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants