Add CIBW_BEFORE_TEST to customize test environment #242

Czaki · 2020-01-07T08:15:19Z

I need this change to overwrite some environment variable (use clang compiler instead of gcc on MacOS)
It allow also use much more elegant solution than

CIBW_TEST_REQUIRES: "scikit_build; pip install -r requirements_test.txt"

to perform multi step requirements installation (in this case it is needed because there is no wheel for python 3.8).

I think that it is better solution than #240 which is my first try to solve this problem.

YannickJadoul · 2020-01-10T11:20:43Z

Restarted the test (cfr. #243 (comment)).

I still need to think about this and review this, but I don't have time, right now. I'll get a look somewhere next week or so.

Czaki · 2020-01-10T11:25:56Z

One additional remark. From PR #164 test are executed in separated environment. So CIBW_BEFORE_BUILD does not allow to modify test environment.

joerick · 2020-01-10T18:05:00Z

As noted in #243 , I'm +1 on this, in principle. Is this ready for review now @Czaki ?

Czaki · 2020-01-10T18:07:02Z

@joerick Yes

joerick

See my discussion on use-cases, I'm a little confused on what actual real-world use of the option would look like!

docs/options.md

joerick · 2020-01-10T18:30:32Z

docs/options.md

+A shell command to run after setup test environment. This option allows you to run a command in **each** Python environment before the `pip wheel` command. This is useful if you need to install non pip package, change values of environment variables
+or perform multi step pip installation (like install `scikit-build` or `cython` before install test package)


These reasons are a little curious, actually.

Non-pip packages don't care about the virtualenv, so can be installed in BEFORE_BUILD.

Changing environment variables won't work, since the command is run in a subshell

Maybe I'm missing something, but I can't see the need to install anything before installing the wheel? Wheel installation is just an unzip and move, no code within the package is run. So why is there a need to install dependencies before that? It seems that TEST_REQUIRES would be sufficient for these cases.

Example situation:

PyQt5 from 5.14.0 has wheel with manylinux2014 platform tag. Earlier are published with manylinux1. manylinux2014 needs pip in version 19.3. Example solution with BEFORE_BUILD is to manually install PyQt5==5.13.2

cmake package does not provide wheels for x86 python 3.8. So only option is to build it from source. But cmake package needs scikit-build for build and this dependency needs to be importable (installed in previous transatction). And package blosc does not provide wheel and need cmake package to be installed. And tests need blosc package.

i do not have given example but it is possible to have similar story with Cython

Also non pip package can care about virtualenv (location of python executable). Here example: https://github.com/LLNL/zfp

And change environment variable I think about ENV1=1 ENV2=7 command

docs/options.md

joerick · 2020-01-10T18:31:51Z

docs/options.md

+CIBW_BEFORE_TEST: CC=gcc CXX=g++ pip install -r requirements.txt
+
+# chain commands using &&
+CIBW_BEFORE_TEST: yum install -y libffi-dev && pip install .


pip install . would be an anti-pattern, since it would build and install from code, rather than from the wheel.

Agree. I copy it from CIBW_BEFORE_BUILD documentation. Whole example is bad. Installig things system side during testis also bad idea. Maybe pip install scikit-build && pip install cmake?

How about this?

# chain commands using && CIBW_BEFORE_TEST : rm -rf ./data/cache && mkdir -p ./data/cache

joerick · 2020-01-10T18:32:09Z

docs/options.md

+#### Examples
+```yaml
+# install packages needed to build test dependencies
+CIBW_BEFORE_TEST: pip install cmake scikit-build


See above, I'm not sure why these aren't TEST_REQUIRES

As I mentioned before. Some dependencies need to be importable in setup.py. And still some authors of packages does not provide wheels or it takes over month since release of python 3.8 to wheel for this python. And such things allow maintainer to have full test case without waiting on other persons.

From one of my build

File "/tmp/pip-install-f9rmc9hk/cmake/setup.py", line 7, in <module> from skbuild import setup ModuleNotFoundError: No module named 'skbuild'

@joerick I know setup_requires doesn't always work, for some reason. So that would be good.

But indeed, this might not be the perfect example to put in the docs, either?

I see. Maybe it shouldn't be the first example. Could we move it so it's the last one, and change comment to # install python packages that are required to install test dependencies

joerick · 2020-01-10T18:33:40Z

docs/options.md

+CIBW_BEFORE_TEST: pip install cmake scikit-build
+
+# install test dependencies with overwritten environment variables. 
+CIBW_BEFORE_TEST: CC=gcc CXX=g++ pip install -r requirements.txt


Curious - why would this be necessary?

My case is that i want to build package with OMP support for MacOS. MacOS clang does not support this. So I need to use brew g++ for this. But one of test dependencies need to be compiled with system clang. Otherwise I got such message:

python(53258,0x7fff9481d380) malloc: *** error for object 0x10f526c00: pointer being freed was not allocated *** set a breakpoint in malloc_error_break to debug

Other case which I can imagine is that my package is build against newest version of some library. And test dependency is build against older (system one). And I need to change LD_LIBRARY_PATH etc to avoid collision.

This confusion makes me wonder; wouldn't a different example be better for the docs, then?

What do you think about
CFLAGS="-I ... -L ..." pip install package name
for custom library location?

Huh, maybe. But .. that's again something where you'd ask the question where a user would ask why this is not CIBW_TEST_ENVIRONMENT...

Totally different example. Install non pip package like: https://github.com/LLNL/zfp

For this... (original example)
MacOS system clang does not support OpenMP. So if package needs OpenMP support then there is need to compile it with gcc/g++ (ex from brew).

For second. I meet problem when test for one package fail on Azure Pipeline but not fail on my computer. And this fail was connected with package used in test and build from source. And for me possibility to control compilation of single package is big profit.

I think this one is okay as-is, the comment makes sense. (I agree about CIBW_TEST_ENVIRONMENT @YannickJadoul, we can remove this example if that appears)

test/10_before_test/cibuildwheel_test.py

YannickJadoul · 2020-01-16T15:38:22Z

cibuildwheel/windows.py

@@ -58,7 +58,7 @@ def get_python_configurations(build_selector):
    return python_configurations


-def build(project_dir, output_dir, test_command, test_requires, test_extras, before_build, build_verbosity, build_selector, repair_command, environment):
+def build(project_dir, output_dir, test_command, test_requires, test_extras, before_build, build_verbosity, build_selector, repair_command, environment, before_test):


Ugh, these arguments are becoming a mess.

At any rate, I don't think before_test should be alone at the end of the argument list. Can you put it maybe after test_command or so?

Agree. But there is question if use kwargs or reorder arguments? @joerick ?

I'd say just reorder now such that it make +- logical sense. Refactoring this (maybe **kwargs, but I don't know what the best solution is) is for another time/PR.

README.md

test/10_before_test/cibuildwheel_test.py

docs/options.md

joerick · 2020-01-18T16:44:37Z

docs/options.md

+#### Examples
+```yaml
+# install packages needed to build test dependencies
+CIBW_BEFORE_TEST: pip install cmake scikit-build


I see. Maybe it shouldn't be the first example. Could we move it so it's the last one, and change comment to # install python packages that are required to install test dependencies

joerick · 2020-01-18T16:49:45Z

docs/options.md

+CIBW_BEFORE_TEST: pip install cmake scikit-build
+
+# install test dependencies with overwritten environment variables. 
+CIBW_BEFORE_TEST: CC=gcc CXX=g++ pip install -r requirements.txt


I think this one is okay as-is, the comment makes sense. (I agree about CIBW_TEST_ENVIRONMENT @YannickJadoul, we can remove this example if that appears)

joerick · 2020-01-18T16:57:00Z

docs/options.md

+CIBW_BEFORE_TEST: CC=gcc CXX=g++ pip install -r requirements.txt
+
+# chain commands using &&
+CIBW_BEFORE_TEST: yum install -y libffi-dev && pip install .


How about this?

# chain commands using && CIBW_BEFORE_TEST : rm -rf ./data/cache && mkdir -p ./data/cache

test/10_before_test/cibuildwheel_test.py

test/10_before_test/setup.py

joerick · 2020-01-18T17:05:25Z

test/10_before_test/setup.py

+# assert that the Python version as written to pythonversion.txt in the CIBW_BEFORE_BUILD step
+# is the same one as is currently running.
+version_file = 'c:\\pythonversion.txt' if sys.platform == 'win32' else '/tmp/pythonversion.txt'
+if os.path.exists(version_file):
+    os.remove(version_file)
+
+# check that the executable also was written
+executable_file = 'c:\\pythonexecutable.txt' if sys.platform == 'win32' else '/tmp/pythonexecutable.txt'
+if os.path.exists(executable_file):
+    os.remove(executable_file)


Just thinking- I'm not sure we need these delete steps here in setup.py..?

I agree. The reason these were in CIBW_BEFORE_BUILD is probably because only setup.py gets run here. Now there's a test suite that's run, so things should be checked there.

In macos build cibuildwheel use symlink for choosing python versions. Then all python version has same executable path. So we need to delete this files to be sure that CIBW_BEFORE_BUILD is executed properly.

This is an excellent point about pythonversion.txt. I previously said that pythonversion.txt should be removed in this test - I was wrong, for the symlink reason above, we should keep that. Apologies @Czaki!

However, I still think the setup.py 'remove files' step is unnecessary. Each test will overwrite the previous version.

I revert this change and remove cleaning.

unit_test/main_options_test.py

YannickJadoul · 2020-01-18T17:17:39Z

test/10_before_test/setup.py

+# assert that the Python version as written to pythonversion.txt in the CIBW_BEFORE_BUILD step
+# is the same one as is currently running.
+version_file = 'c:\\pythonversion.txt' if sys.platform == 'win32' else '/tmp/pythonversion.txt'
+if os.path.exists(version_file):
+    os.remove(version_file)
+
+# check that the executable also was written
+executable_file = 'c:\\pythonexecutable.txt' if sys.platform == 'win32' else '/tmp/pythonexecutable.txt'
+if os.path.exists(executable_file):
+    os.remove(executable_file)


I agree. The reason these were in CIBW_BEFORE_BUILD is probably because only setup.py gets run here. Now there's a test suite that's run, so things should be checked there.

test/10_before_test/cibuildwheel_test.py

test/10_before_test/test/spam_test.py

Czaki · 2020-01-19T23:13:21Z

I'm a little lost with comments to documentation. I try to apply all, but I'm not sure if I properly understand all suggestions. Could You check?

joerick

Looks good to me! @YannickJadoul any further comments/questions or are we good to merge?

joerick · 2020-02-28T16:14:32Z

Would be nice to get this into the next release - @Czaki could you merge/rebase please? @YannickJadoul did you have any further comments before it goes in?

YannickJadoul

Sorry, I'd missed reviewing the new version!

Found one more little thing, I think.
And the test probably needs a new number, because by now we already have a test nr. 10.

README.md

Czaki · 2020-02-28T23:53:41Z

@YannickJadoul again pypy problem with virtualenv separation.
From travis windows:

which python

/tmp/tmpeoj7ehk3/Scripts/python

+ pip install C:\Users\travis\AppData\Local\Temp\cibuildwheel4ut18dye\repaired_wheel\spam-0.1.0-pp36-pypy36_pp73-win32.whl

Requirement already satisfied: spam==0.1.0 from file:///C:/Users/travis/AppData/Local/Temp/cibuildwheel4ut18dye/repaired_wheel/spam-0.1.0-pp36-pypy36_pp73-win32.whl in c:\cibw\pypy3.6-v7.3.0-win32\site-packages (0.1.0)

YannickJadoul · 2020-02-28T23:59:54Z

@Czaki Sigh :-( But this time on Windows? I'll have a look tomorrow, if I can. Could you meanwhile test if virtualenv<20 works?

Czaki · 2020-02-29T00:03:52Z

I think that 02_test should be somehow rewritten to check if is running in virtualenv...

Lest wait on result.

Czaki · 2020-02-29T16:33:00Z

@YannickJadoul Yes. Again problem with virtualenv.

Travis is somehow buggy.

write /var/lib/docker/tmp/GetImageBlob188542173: no space left on device

YannickJadoul · 2020-02-29T17:05:30Z

@YannickJadoul Yes. Again problem with virtualenv.

Args. Sorry, today I don't have time to look at it. Maybe tomorrow, I don't know.
Do you think you can try make a minimal example that shows the error? virtualenv 20 is still young, so if you can show what's happening, maybe it could be a virtualenv issue.

Is it also an issue on master, btw, or is it linked to these changes?

Travis is somehow buggy.

write /var/lib/docker/tmp/GetImageBlob188542173: no space left on device

I restarted everything in the latest build that failed.

Czaki · 2020-02-29T17:09:56Z

I restarted everything in the latest build that failed.
Again same behaviour. I think that this need to wait till Monday when person from Travis goes back to work.

Is it also an issue on master, btw, or is it linked to these changes?

There is no test in master that cover this case...

I will think about this and maybe produce some test which maybe can be used as example.

Czaki · 2020-03-01T08:56:37Z

@YannickJadoul Done

Co-Authored-By: Joe Rickerby <joerick@mac.com>

Czaki · 2020-03-07T14:44:53Z

@joerick Ready for merge. appveyor queue is rely long.

test/11_before_test/test/spam_test.py

Czaki · 2020-03-07T17:48:52Z

Not ready. Need to somehow fix this. What you think about compare folders instead of executable (pypy3.exe vs python.exe)

joerick · 2020-03-07T17:56:49Z

what's the difference between the paths? is python.exe a symbolic link to pypy3.exe?

joerick · 2020-03-07T17:59:55Z

ah, yes it is, cibuildwheel creates it when it installs pypy.

this might help

joerick · 2020-03-07T18:17:42Z

if that doesn't work, you could consider a hard link from python.exe to pypy3.exe in install_pypy, instead of a symbolic one - maybe os.stat would return the same in that case?

Czaki · 2020-03-08T12:12:01Z

if that doesn't work, you could consider a hard link from python.exe to pypy3.exe in install_pypy, instead of a symbolic one - maybe os.stat would return the same in that case?

It is not so easy. this executables are cleated by virtualenv, not by instal_pypy And both files are not links.
Possible solutions which I see:

check if bin dir is same (os.path.dirname)
use sys.prefix link instead of sys.executable
use information form sys.implementation and calculate name of executable?

joerick · 2020-03-08T13:08:06Z

Ah, I hadn't realised that it was virtualenv creating the executables!

hmm. hmm! Of the 3 choices above, my preference would be to compare sys.prefix, instead.

Czaki · 2020-03-08T15:22:52Z

So ready.

Czaki · 2020-03-08T15:36:04Z

Hm. Using circleci and appveyor really slow testing PR :(

Czaki force-pushed the before_test branch from 5206be7 to bb369ff Compare January 9, 2020 07:29

Czaki mentioned this pull request Jan 10, 2020

install order in test enviroment #243

Closed

joerick reviewed Jan 10, 2020

View reviewed changes

Czaki mentioned this pull request Jan 16, 2020

Deterministic builds #239

Closed

YannickJadoul reviewed Jan 16, 2020

View reviewed changes

joerick reviewed Jan 18, 2020

View reviewed changes

joerick changed the title ~~Add before test step to customize test enviroment~~ Add CIBW_BEFORE_TEST to customize test environment Jan 18, 2020

YannickJadoul reviewed Jan 18, 2020

View reviewed changes

Czaki force-pushed the before_test branch 2 times, most recently from 9ccdc79 to 586cac8 Compare January 19, 2020 23:03

joerick approved these changes Jan 31, 2020

View reviewed changes

YannickJadoul reviewed Feb 28, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

Czaki force-pushed the before_test branch 4 times, most recently from dbbb683 to 7852971 Compare February 28, 2020 23:03

Czaki force-pushed the before_test branch from ed8c358 to 4a4d58b Compare February 29, 2020 15:43

Czaki force-pushed the before_test branch from 4a4d58b to 81584c0 Compare March 1, 2020 08:14

Czaki mentioned this pull request Mar 1, 2020

change 02_test to check if virtualenv is properly used #282

Closed

Czaki and others added 6 commits March 7, 2020 12:24

add before test variable

4e93160

add documentation

d9e6bbf

Apply spell suggestions from joerick code review

6461074

Co-Authored-By: Joe Rickerby <joerick@mac.com>

reorder before_test

01a7c2d

remove clean files from setup.py, fixes in doc

d613a8c

add example with non pip python package

baba770

Czaki force-pushed the before_test branch 2 times, most recently from 5191dc0 to 4ec06df Compare March 7, 2020 13:35

joerick linked an issue Mar 7, 2020 that may be closed by this pull request

install order in test enviroment #243

Closed

joerick reviewed Mar 7, 2020

View reviewed changes

test/11_before_test/test/spam_test.py Outdated Show resolved Hide resolved

fix test after rebase, fix typos in readme

79cf7e4

Czaki force-pushed the before_test branch from 4ec06df to 79cf7e4 Compare March 7, 2020 17:18

uses sys.prefix instead of sys.executable in before_test

7e566e4

joerick merged commit 4caee57 into pypa:master Mar 8, 2020

Czaki deleted the before_test branch March 24, 2020 17:23

		A shell command to run after setup test environment. This option allows you to run a command in each Python environment before the `pip wheel` command. This is useful if you need to install non pip package, change values of environment variables
		or perform multi step pip installation (like install `scikit-build` or `cython` before install test package)

Add CIBW_BEFORE_TEST to customize test environment #242

Add CIBW_BEFORE_TEST to customize test environment #242

Conversation

Czaki commented Jan 7, 2020

YannickJadoul commented Jan 10, 2020

Czaki commented Jan 10, 2020

joerick commented Jan 10, 2020

Czaki commented Jan 10, 2020

joerick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki Jan 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Czaki commented Jan 19, 2020

joerick left a comment

Choose a reason for hiding this comment

joerick commented Feb 28, 2020

YannickJadoul left a comment

Choose a reason for hiding this comment

Czaki commented Feb 28, 2020

YannickJadoul commented Feb 28, 2020

Czaki commented Feb 29, 2020

Czaki commented Feb 29, 2020 • edited Loading

YannickJadoul commented Feb 29, 2020

Czaki commented Feb 29, 2020

Czaki commented Mar 1, 2020

Czaki commented Mar 7, 2020

Czaki commented Mar 7, 2020

joerick commented Mar 7, 2020

joerick commented Mar 7, 2020

joerick commented Mar 7, 2020

Czaki commented Mar 8, 2020

joerick commented Mar 8, 2020

Czaki commented Mar 8, 2020

Czaki commented Mar 8, 2020

Czaki Jan 10, 2020 •

edited

Loading

Czaki Jan 19, 2020 •

edited

Loading

Czaki commented Feb 29, 2020 •

edited

Loading