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

Proper config for building from sdist using github action #1125

Closed
ajfriend opened this issue Jun 4, 2022 · 4 comments
Closed

Proper config for building from sdist using github action #1125

ajfriend opened this issue Jun 4, 2022 · 4 comments

Comments

@ajfriend
Copy link

ajfriend commented Jun 4, 2022

Description

Hi. I'm trying to use the feature added in #1096 to build from SDists using the pypa/cibuildwheel GitHub Action.

I'm currently working on it in this PR: uber/h3-py#258

Main question (tests currently not working)

What's the idiomatic/correct way to set up the CIBW_TEST_COMMAND variable when building from the SDist?

Neither of the following seem to work:

CIBW_TEST_COMMAND: pytest {project}/tests
CIBW_TEST_COMMAND: pytest h3-py/tests

I'm assuming that's because the {project} variable is set differently for the SDist case.

Minor idiom question

I'm building the SDist in a separate GA job, and downloading the artifact for the CIBW job. Does anyone see a cleaner/safer way than the glob pattern to specify the SDist?

      - uses: actions/download-artifact@v3
        with:
          name: artifact
          path: dist

      - uses: pypa/cibuildwheel@v2.6.0
        with:
          package-dir: dist/h3-*.tar.gz
          output-dir: dist

Build log

No response

CI config

No response

@ajfriend
Copy link
Author

ajfriend commented Jun 6, 2022

I suppose the issue stems from the fact that we don't include a tests folder in our sdist. In that case, what's the right way to escape out of the sdist directory to access the tests in the project root?

@joerick
Copy link
Contributor

joerick commented Jun 7, 2022

What's the idiomatic/correct way to set up the CIBW_TEST_COMMAND variable when building from the SDist?

The pytest {project}/tests form is the idiomatic way. The {project} is required, because the tests are run from a different directory to ensure that the installed wheel is picked up when you import <library>, not the repo version.

I suppose the issue stems from the fact that we don't include a tests folder in our sdist.

Correct. The cibuildwheel process changes directory to the expanded sdist to run the build, so paths that aren't within the sdist won't work. There is a note about this hidden in the --help documentation,

(PACKAGE section)
 When set to a tar.gz sdist file, --config-
file and --output-dir are relative to the current
directory, and other paths are relative to the
expanded SDist directory.

In that case, what's the right way to escape out of the sdist directory to access the tests in the project root?

There isn't a supported way. When building like this, the sdist is considered the 'project' and is the unit that's copied into the Docker container. All option Some people advocate including tests in the sdist for this sort of reason.

There is a workaround, you can use an absolute path to your tests in the option. On Linux, prepend the path with /host/ to use the Docker mount. The envvar $GITHUB_WORKSPACE might be useful.

@ajfriend
Copy link
Author

ajfriend commented Jun 9, 2022

Thanks. This makes sense.

It sounds like my options are:

  1. Just keep building from src instead of the SDist.
  2. Include tests in the SDist.

I'll figure out what works best for our project. :)

Long term, would you be willing to consider a feature request to allow users to point to tests outside of the sdist?

@joerick
Copy link
Contributor

joerick commented Jun 17, 2022

Yeah, that sounds right to me.

Long term, would you be willing to consider a feature request to allow users to point to tests outside of the sdist?

Yes, in principle. In practice, it might be tricky, given that we'd have to ensure that it was accessible inside the Docker container (the /host/ mount has always been more of an escape hatch than a real feature, because it's possible that your Docker daemon isn't running on the same machine that cibuildwheel is, CircleCI is an example of this).

@joerick joerick closed this as completed Jun 17, 2022
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

No branches or pull requests

2 participants