TST: simplify pep518 test setup #212
Conversation
17a650a to
2592309
Compare
11fd293 to
bdd6376
Compare
|
@henryiii I took the freedom to tweak the pep518 test setup in the way I had in mind. Please have a look. I think this is easier. I still don't understand why |
bdd6376 to
b9d5d40
Compare
Agreed, this is not great. The intent is micro-optimization of not installing things in the build env when they are not needed for isolated builds. However, that breaks non-isolated builds - which the kind of projects that are moving to Meson need all the time. So it is more harmful than helpful. Maybe let's move that to a separate PR or issue? |
|
That makes sense. We may be moving away from using |
| @@ -15,10 +15,9 @@ | |||
| @pytest.mark.parametrize( | |||
| 'build_arg', ['', '--wheel'], ids=['sdist_to_wheel', 'wheel_directly'] | |||
| ) | |||
| @pytest.mark.xfail(sys.platform.startswith('cygwin'), reason='ninja pip package not available on cygwin', strict=True) | |||
There was a problem hiding this comment.
Why is this removed? The point of the test is to test that this breaks - that is, ninja's not available from PyPI on cygwin. So if this passes on Cygwin before #175 goes in, the test doesn't work. That's why it was a strict xfail.
I'm not completely sure what changed to make this pass, as this looks similar.
There was a problem hiding this comment.
Okay, now I know why it's passing. That's okay, I think - then we can delete ninja from the wheelhouse manually for #175. The only think that lightly bothers me is the check_call change, then.
There was a problem hiding this comment.
I think that if we remove ninja from meson-python's dependencies list it will not be put in the wheelhouse anymore. But the documentation is not crystal clear on this. I'll have to check.
There was a problem hiding this comment.
I confirm that once ninja is removed from meson-python's dependencies it is not added to the wheelhouse anymore.
tests/test_pep518.py
Outdated
| subprocess.run([sys.executable, '-m', 'build', f'--outdir={dist}', *build_args], cwd=package_dir, check=True) | ||
| subprocess.check_call([sys.executable, '-m', 'build', '--outdir', str(dist), *build_args]) |
There was a problem hiding this comment.
Why are you replacing run with check_call? check_call and check_output are the old, pre Python 3.5 API, and the Python docs recommend run instead. The rest of the calls should be changed to run long-term, I think, not the other way around?
There was a problem hiding this comment.
Oh, sorry, I saw that the rest of the code uses the "old" API and I assumed it is the recommended one. Switching it back and probably adding a commit to clean up the other call sites too.
| meson_python = str(package_dir.parent.parent) | ||
| # Populate wheelhouse with wheel for the following packages and | ||
| # their dependencies. Wheels are downloaded from PyPI or built | ||
| # from the source distribution as needed. Sources or wheels in |
There was a problem hiding this comment.
Ahh, it's building ninja from source on Cygwin here, that's why it's passing below. In my version, it was also building ninja from source in order to build meson, but it wasn't putting it into the wheelhouse.
|
Listing wheel as a dependency until recently pulled in setuptools. |
This also makes the test work on Cygwin where a ninja wheel is not available and needs to be built from the source distribution.
bcbbd80 to
8d88f2c
Compare
8d88f2c to
427b1fc
Compare
No description provided.