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

Edit path when building sdist package #1687

Merged
merged 4 commits into from Jan 24, 2024

Conversation

OlenaYefymenko
Copy link
Contributor

@OlenaYefymenko OlenaYefymenko commented Dec 5, 2023

This patch changes the working directory from the temp to the project when building sdist package. This resolves issues with relative paths in configuration files.

Resolves #1683

This patch changes the working directory from the temp to the project
when building sdist package.This resolves issues with relative paths
in configuration files.

Resolves pypa#1683
@joerick
Copy link
Contributor

joerick commented Dec 6, 2023

Nice one, thanks! Could you also add an integration test for this? It might be possible to add an assert to an existing test in

https://github.com/pypa/cibuildwheel/blob/main/test/test_from_sdist.py

@webknjaz
Copy link
Member

webknjaz commented Dec 6, 2023

@joerick TBH it's not exactly obvious what kind of check could be added into cibuildwheel_from_sdist_run. Any clues? The internal CWD is only accessible from within a subprocess, right? Wouldn't that require something like an in-tree build back-end that would save that dir somewhere accessible externally?

@joerick
Copy link
Contributor

joerick commented Dec 6, 2023

CIBW_BEFORE_BUILD runs from the CWD, so we could check we are in the project dir by doing something like CIBW_BEFORE_BUILD=python3 -c 'import os; assert os.path.exists("setup.py")'.

@webknjaz
Copy link
Member

UPD: adding tests may take a while since Olena is a beginner and needs to learn a thing or two about the ecosystem. I was helping her out to figure out how to run tests locally and learned that following the CONTRIBUTING instructions doesn't work — the way the tests are being executed requires something like GITHUB_ACTIONS=true nox -s tests to pretend the env is a CI. Otherwise, the underlying invocation of cibuildwheel in tests errors out with a return code of 2 and an error message saying that it couldn't detect a CI.

@joerick have you considered synchronizing the CIs with the way the invocations are supposed to run locally to avoid such discrepancies? As in run nox everywhere the same (ish) way? One interesting approach I saw in some other projects was having a dedicated "dev env" CI job that essentially runs the tools in a way the contributors would do it locally.

@joerick
Copy link
Contributor

joerick commented Dec 13, 2023

Apologies, that doc does probably need a note about this. I wouldn't recommend setting the GITHUB_ACTIONS like you mention, rather what I do is set CIBW_PLATFORM e.g. CIBW_PLATFORM=macos or CIBW_PLATFORM=linux (both run locally on my Mac, because the linux one runs in docker).

I don't use the nox integration personally, because the test suite takes a while to run, so I generally want to be more specific. I will generally do pytest unit_test or if I want to run an integration test, I might do CIBW_PLATFORM=linux pytest test -k <test_name>.

Just one caveat to the above - for local runs with CIBW_PLATFORM=macos, your system might be missing some versions of Python - you can install them from Python.org, the error message should include the link to download.

@joerick
Copy link
Contributor

joerick commented Dec 13, 2023

@joerick have you considered synchronizing the CIs with the way the invocations are supposed to run locally to avoid such discrepancies? As in run nox everywhere the same (ish) way? One interesting approach I saw in some other projects was having a dedicated "dev env" CI job that essentially runs the tools in a way the contributors would do it locally.

Interesting idea... though really the problem here is that the docs are incomplete, I don't think we can test for that.

@webknjaz
Copy link
Member

I don't use the nox integration personally, because the test suite takes a while to run, so I generally want to be more specific. I will generally do pytest unit_test or if I want to run an integration test, I might do CIBW_PLATFORM=linux pytest test -k <test_name>.

@joerick I checked the noxfile.py and it does allow you to pass arbitrary args to pytest. So you can do nox -s tests -- unit_test or nox -s tests -- test -k <test_name>. This is a commonly known approach that is usually implemented with tox too. Could you try it out? This should eliminate the need for you to run pytest directly.

@webknjaz
Copy link
Member

@joerick have you considered synchronizing the CIs with the way the invocations are supposed to run locally to avoid such discrepancies? As in run nox everywhere the same (ish) way? One interesting approach I saw in some other projects was having a dedicated "dev env" CI job that essentially runs the tools in a way the contributors would do it locally.

Interesting idea... though really the problem here is that the docs are incomplete, I don't think we can test for that.

The idea with workflows tools like tox / nox is that you absolutely can make the invocations nearly the same locally and in CI without having to compromise the ability to invoke the underlying tools with customizations. This is why the posargs supplied after -- are usually bypassed to those integrated invocations in the respective configs.

Note that both tools have ways to list the configured sessions. They are tox -av or nox --list — and it's a convenient centralized place to document what different sessions implements. For tox, there's a dedicated setting for the env description, and in the case of nox, these are taken from docstrings.

If the contributing doc says to check out the sessions in the workflow tool, with a few more concrete examples, and the CI runs the same, that's usually the best way of making sure there's no difference between what different people run.

A more sophisticated solution could be auto-generating a portion of the contributing doc from the nox file.

@henryiii
Copy link
Contributor

FYI, you can also use CIBW_PLATFORM=linux nox -Rs tests -- test -k <test_name> which will avoid reinstalling anything and be just as fast as running it manually (at least the second time it runs).

@joerick
Copy link
Contributor

joerick commented Dec 13, 2023

I've just pushed a PR #1698 with an improvement to this page. My opinion is that the direct pytest invocation is better, as I'm nearly always using some pytest-specific option, so it's extra level of indirection and mental overhead to also think about the nox layer, and a little confusing to thing about passing options to nox that behind the scenes get passed through to pytest. Also, that's the method I always use, so I'm more likely to notice if it changes.

Regarding using nox in CI, it's complicated by the fact that we do a few tricks in CI (like CI-specific flags for docker/podman checks, parallel execution) that we probably wouldn't pass on to the end-user. Perhaps those features from bin/run_tests.py could be added to the nox task. That might be okay, but I'm more of the opinion that the problem here is just missing documentation.

@webknjaz
Copy link
Member

@joerick while I understand your sentiment, people are quite used to passing arguments to the wrapped commands. This is quite idiomatic in the tox world (and by extension, nox too). The whole idea is that this ends up always using the correct environment that is set up properly and the same way as in the CI. Tox also has --devenv for exposing disconnected envs, not sure nox has the same.
Either way, you stated that youdo things this way, which is fine because you're proficient and know enough implementation details. But it's hugely different for an arbitrary contributor. This is why, their starting point should be as close to the CI as possible. This doesn't prevent folks from doing advanced stuff but lowers the entry barrier for the beginners, which I think is important.

As for the custom CLI options, a better place for them is pytest. I can take a look into that, if that's something you'd like to explore.

The change extends the existing test — `test_simple`. It checks that
the temporary wheel build directory is the project root extracted from
the source distribution. It does so by testing the presence of the
`setup.py` in the current working directory, as a side effect.
test/test_from_sdist.py Outdated Show resolved Hide resolved
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
test/test_from_sdist.py Outdated Show resolved Hide resolved
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
@webknjaz
Copy link
Member

@henryiii @joerick approve the GHA run?

@webknjaz
Copy link
Member

No CI failures so far 🤞 Waiting for the completion of the rest of the jobs.

@webknjaz
Copy link
Member

@joerick @henryiii the CI is fully green. Would you like some commits to be polished before merging?

@henryiii
Copy link
Contributor

Nah, squash and merge is fine unless files both move and change.

@henryiii henryiii merged commit a1e3efb into pypa:main Jan 24, 2024
18 checks passed
@joerick
Copy link
Contributor

joerick commented Jan 26, 2024

Thanks all for this!

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.

[FR] Expose {package} placeholder to the build stage
4 participants