Fix cwd in Linux docker images #410

YannickJadoul · 2020-07-19T22:47:19Z

Closes #409

YannickJadoul · 2020-07-19T23:46:51Z

Think we're almost there. Questions left:

Is this fine, adding cwd to the constructor, or should we add cwd to each docker.call?
More tests? Maybe CIBW_BEFORE_ALL as well? And anything else to test?
Is there a better way of adding this test? We could just add another test function to test_before_build and not go through Python? Also, can we compare stored_cwd to something else for Windows/macOS?

YannickJadoul · 2020-07-20T00:18:02Z

Broken because this issue is fixed, but #411 isn't.

test_overridden_path's 'CIBW_BEFORE_ALL': 'mkdir new_path && touch new_path/python && chmod +x new_path/python' is now also evaluated from /project, but $(pwd) in ``'CIBW_ENVIRONMENT': '''PATH="$(pwd)/new_path:$PATH"'''is not interpolated correctly, so/new_path` does not contain `python`, so the Python executable on `PATH` is not overwritten.

I'll rebase onto #411.

…nd confirm it is not

YannickJadoul · 2020-07-20T16:08:21Z

(rebased after merge of #411)

joerick

Thanks again for looking at these fixes, as I don't have time just at the moment!

joerick · 2020-07-20T18:30:47Z

cibuildwheel/linux.py

+            with DockerContainer(docker_image, simulate_32_bit=platform_tag.endswith('i686'), cwd='/project') as docker:
+                docker.copy_into(Path.cwd(), PurePath('/project'))


Setting cwd to /project in the constructor feels a little strange, because I don't think /project has been created yet. I suppose Docker must create it if it's not there. But doesn't feel right to me. I think a more familar approach might be to have docker.chdir(PurePath('/project')) after the project is copied over.

The cwd can be stored in DockerContainer as an ivar, and used whenever call() is called without a cwd. What do you think?

Docker does document that passing -w will create the directory: https://docs.docker.com/engine/reference/commandline/create/#options

For the DockerContainer, I thought of it as just setting the "default directory" during the with-statement, and just adding an exception if necessary. And also that we then couldn't forget it, later.

But yes, chdir would also work, if you prefer that. I hadn't thought of that option. I guess we don't even need to store it, but could just cd, rather than storing it as an attribute?

Ah, that's good that's it's documented. Then I think either approach is valid, actually. Happy to leave it as-is.

Given that we don't have a os.chdir for macOS or Windows (and just use the working directory that's there at the start of cibuildwheel/build), I think the current approach might match better. But I'm happy changing it as well, if you prefer. I like the suggested chdir method as well (and we might end up needing it at some later point?). Your call!

@Czaki, any input on this? Do you prefer with DockerContainer(..., cwd='/project'): or with DockerContainer(...) as docker: docker.chdir('/project')?

I prefer with DockerContainer(..., cwd='/project'): and change second argument of

def copy_into(self, from_path: Path, to_path: Optional[PurePath]=none) if to_path is None: to_path = self.cwd ...

Oh, that's an interesting suggestion. I'll add that a as extra commit. If @joerick doesn't like it, it can just be removed before merging!

Actually, that's not possible, @Czaki, because self.cwd can also be None. Do you suggest any way to fix it, or should we just keep to_path in copy_into mandatory?

Raise exception with information that cwd should be set or to_path.
I suggest this to reduce number of places where path is provided as literal.

Yeah, possible, but ... it's still used in a lot of places, like format(..., project='/project', ...), so doing this won't solve that all?

joerick · 2020-07-20T18:37:57Z

test/test_before_build.py

+
+        if sys.platform == 'linux':
+            cwd_file = '/tmp/cwd.txt'
+            with open(cwd_file) as f:
+                stored_cwd = f.read()
+            assert stored_cwd == '/project'


Looks like this is missing a mac and windows test. Also, it might be a bit Rube Goldberg - could you instead assert os.getcwd() == '/project'? And in BEFORE_ALL, do the same?

Looks like this is missing a mac and windows test.

Well, yes, but I didn't know what directory they're supposed to be executed from? As far as I know, it will depend on the CI; probably somewhere in the home directory or a subdirectory?

Also, it might be a bit Rube Goldberg - could you instead assert os.getcwd() == '/project'?

Nope, that's not working. The tests are actually executed from a different directory, and we want to test CIBW_BEFORE_BUILD is executed from the right directory. Or do you mean to python -c "import os; os.getcwd() == '/project'" in CIBW_BEFORE_BUILD?

And in BEFORE_ALL, do the same?

Yes, will do. But I'll wait until the above gets settled.

Or do you mean to python -c "import os; os.getcwd() == '/project'" in CIBW_BEFORE_BUILD?

Yeah, that's what I was thinking, I suppose it'd be something like...

{ 'CIBW_BEFORE_BUILD': f'''python -c "import os; assert os.getcwd() == {os.getcwd()!r}"''', 'CIBW_BEFORE_ALL': f'''python -c "import os; assert os.getcwd() == {os.getcwd()!r}"''', 'CIBW_BEFORE_BUILD_LINUX': '''python -c "import os; assert os.getcwd() == '/project'"''', 'CIBW_BEFORE_ALL_LINUX': '''python -c "import os; assert os.getcwd() == '/project'"''', }

Windows on GH Actions uses /d/a/bls-signatures/bls-signatures which is the repo name twice: https://github.com/Chia-Network/bls-signatures/runs/887873992?check_suite_focus=true#step:10:60

Mac is /Users/runner/work/bls-signatures/bls-signatures/ see: https://github.com/Chia-Network/bls-signatures/runs/887873983?check_suite_focus=true#step:11:535

Windows on GH Actions uses /d/a/bls-signatures/bls-signatures which is the repo name twice: https://github.com/Chia-Network/bls-signatures/runs/887873992?check_suite_focus=true#step:10:60

Mac is /Users/runner/work/bls-signatures/bls-signatures/ see: https://github.com/Chia-Network/bls-signatures/runs/887873983?check_suite_focus=true#step:11:535

Thanks, but that's different from every CI and every project, even (since it contains the project name), so that doesn't seem feasible to distinguish. Let me try @joerick's plan (maybe with __file__ rather than getcwd?) :-)

OK, like this?

I added a test that checks whether a failing CIBW_BEFORE_BUILD and CIBW_BEFORE_ALL also fail the whole run.

YannickJadoul · 2020-07-21T20:36:25Z

Alright. Next round of review! :-)

Slightly worrying note: we're now at 58 and 59 minutes on Azure Pipelines, for the tests on Windows (one of the two jobs actually timed out before I restarted it) :-/ Not sure why Windows (or the Windows infrastructure at Azure) is twice as slow as Linux...

Czaki · 2020-07-21T21:03:07Z

Slightly worrying note: we're now at 58 and 59 minutes on Azure Pipelines, for the tests on Windows (one of the two jobs actually timed out before I restarted it) :-/ Not sure why Windows (or the Windows infrastructure at Azure) is twice as slow as Linux...

Maybe use timeoutInMinutes: 180 ?

I also observe in my projects that windows is much slower.

… slow Windows jobs

YannickJadoul · 2020-07-21T21:14:36Z

Maybe use timeoutInMinutes: 180 ?

I also observe in my projects that windows is much slower.

Alright, thanks! But it's still a bit worrying and very slow to wait for, so ... maybe only a temporary solution :-/

Czaki · 2020-07-21T21:15:51Z

on github acrions and in azure pipelines there is big amount of available workers. Cannot we split tests on two workers?

YannickJadoul · 2020-07-21T21:22:05Z

Yes, maybe. But that feels like a huge mess, as well :-(

Czaki · 2020-07-21T22:22:29Z

How it looks in my project:

https://stackoverflow.com/questions/10150881/why-is-python-so-much-slower-on-windows

Looks like problems with creating multiple process, antivirus, etc, which create bad performance in this case because of multiple spawning of new python using subprocess.

Maybe pytest-xdist?

YannickJadoul · 2020-07-21T22:42:20Z

Maybe pytest-xdist?

I don't know anything about that, but let's discuss in #412 :-)

joerick

Looks good to me! Weird about the Windows test times, but it can't be related to this PR.

YannickJadoul force-pushed the fix-pwd-linux-docker branch 3 times, most recently from b945b54 to a400aa3 Compare July 19, 2020 23:26

YannickJadoul force-pushed the fix-pwd-linux-docker branch from cbfa8c6 to 627e659 Compare July 20, 2020 00:24

YannickJadoul mentioned this pull request Jul 20, 2020

The "/project/" path isn't there anymore for Linux in version 1.5.3 #409

Closed

YannickJadoul added 4 commits July 20, 2020 18:07

Test whether CIBW_BEFORE_BUILD is executed from the right directory a…

2b7da3f

…nd confirm it is not

Add cwd argument to DockerContainer

29b0cdb

Run Linux docker images with /project as cwd

2610f32

Fix test_before_build test on Windows

f81c947

YannickJadoul force-pushed the fix-pwd-linux-docker branch from 627e659 to f81c947 Compare July 20, 2020 16:07

joerick reviewed Jul 20, 2020

View reviewed changes

YannickJadoul force-pushed the fix-pwd-linux-docker branch 6 times, most recently from 26608a5 to f4578eb Compare July 21, 2020 17:55

Extend tests and add tests for CIBW_BEFORE_ALL

c91d669

YannickJadoul force-pushed the fix-pwd-linux-docker branch from f4578eb to c91d669 Compare July 21, 2020 18:12

YannickJadoul requested a review from joerick July 21, 2020 20:36

Make timeout 2 hours on GitHub Actions and Azure Pipelines because of…

9e40094

… slow Windows jobs

joerick approved these changes Jul 22, 2020

View reviewed changes

YannickJadoul merged commit 3cd3a9d into pypa:master Jul 22, 2020

YannickJadoul deleted the fix-pwd-linux-docker branch July 22, 2020 12:41

Czaki mentioned this pull request Jul 22, 2020

use variable instead of '/project' literal #413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cwd in Linux docker images #410

Fix cwd in Linux docker images #410

YannickJadoul commented Jul 19, 2020

YannickJadoul commented Jul 19, 2020 •

edited

Loading

YannickJadoul commented Jul 20, 2020

YannickJadoul commented Jul 20, 2020

joerick left a comment

joerick Jul 20, 2020

YannickJadoul Jul 20, 2020

joerick Jul 20, 2020

YannickJadoul Jul 21, 2020

Czaki Jul 21, 2020 •

edited

Loading

YannickJadoul Jul 21, 2020

YannickJadoul Jul 21, 2020

Czaki Jul 21, 2020

YannickJadoul Jul 21, 2020

joerick Jul 20, 2020

YannickJadoul Jul 20, 2020

joerick Jul 20, 2020

hoffmang9 Jul 21, 2020

YannickJadoul Jul 21, 2020

YannickJadoul Jul 21, 2020 •

edited

Loading

YannickJadoul commented Jul 21, 2020

Czaki commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

Czaki commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

Czaki commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

joerick left a comment

		with DockerContainer(docker_image, simulate_32_bit=platform_tag.endswith('i686'), cwd='/project') as docker:
		docker.copy_into(Path.cwd(), PurePath('/project'))

Fix cwd in Linux docker images #410

Fix cwd in Linux docker images #410

Conversation

YannickJadoul commented Jul 19, 2020

YannickJadoul commented Jul 19, 2020 • edited Loading

YannickJadoul commented Jul 20, 2020

YannickJadoul commented Jul 20, 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

Czaki Jul 21, 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

YannickJadoul Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

YannickJadoul commented Jul 21, 2020

Czaki commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

Czaki commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

Czaki commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

joerick left a comment

Choose a reason for hiding this comment

YannickJadoul commented Jul 19, 2020 •

edited

Loading

Czaki Jul 21, 2020 •

edited

Loading

YannickJadoul Jul 21, 2020 •

edited

Loading