Patching pypy venv #502

Czaki · 2020-12-27T19:15:21Z

Fix for the problem reported in #501

Czaki · 2020-12-28T17:05:08Z

@henryiii @mattip
Could you check this?

I do not understand why windows build pass, maybe my modification of the test is not enough?

mattip · 2020-12-28T17:16:28Z

These changes are only needed for posix.

henryiii · 2020-12-28T18:29:19Z

I think we should have a reminder somewhere that this can be reverted after the next PyPy release. @joerick?

Czaki · 2020-12-28T19:07:15Z

I think we should have a reminder somewhere that this can be reverted after the next PyPy release. @joerick?

one of the problem of this PR is that this code fails when cannot apply the patch. So It will fail when the user does not use pinned images, but will signal that there is need to remove patch after update.

Or I should ignore fail of patch?

henryiii · 2020-12-28T20:01:29Z

Can it check PyPy version?

Czaki · 2020-12-28T20:39:00Z

Can it check PyPy version?

maybe it could parse pypy string.

mattip · 2020-12-28T20:42:18Z

pypy -c "import sys; print(sys.pypy_version_info < (7,3,4))" prints True for versions needing the patch

joerick · 2020-12-29T00:01:05Z

I think we should have a reminder somewhere that this can be reverted after the next PyPy release. @joerick?

Let's open a draft PR that reverts these changes once this is merged, that way we won't forget about it

henryiii · 2020-12-29T05:39:38Z

pypy -c "import sys; print(sys.pypy_version_info < (7,3,4))" prints True for versions needing the patch

You could use a sys.exit return value, but anyway this probably should be the thing the patch triggers on.

Let's open a draft PR that reverts these changes once this is merged, that way we won't forget about it

Most users will use latest versions or the pinned versions, I think, so there would be no reason to still support 7.3.3 after we have a version pinning 7.3.4?

Czaki · 2020-12-29T09:31:18Z

Most users will use latest versions or the pinned versions, I think, so there would be no reason to still support 7.3.3 after we have a version pinning 7.3.4?

I one project I use prebuild docker images because build libraries take ~0.5h. Of course, I will know that I need immediately rebuild docker images, but maybe some other users decide for the same workflow.

joerick · 2020-12-29T12:03:50Z

Ah, right. Well maybe we can leave it around until the next minor point release.

YannickJadoul · 2020-12-29T16:23:15Z

Sorry, @Czaki, I was trying something, but by accident pushed to your branch instead of my local one!

YannickJadoul · 2020-12-29T16:47:12Z

Not sure why GHA wasn't triggered, but this seems to work, and is a dramatic simplification: YannickJadoul@615e905.
Here's the manual GHA run: https://github.com/YannickJadoul/cibuildwheel/runs/1622585851

Mind if I push to your branch, @Czaki, or do you prefer to pull from my branch yourself?

Czaki · 2020-12-29T17:25:13Z

Mind if I push to your branch, @Czaki, or do you prefer to pull from my branch yourself?

-d allows to patch the whole directory?

YannickJadoul · 2020-12-29T17:36:41Z

-d allows to patch the whole directory?

Officially, it says that flag changes to that directory before applying the patch. Then if you only pass one file, that's the patch file, and it will go looking for which files to patch (since they are in the diff, anyway)

henryiii · 2020-12-29T17:40:53Z

Patches are not per-file, add “.patch” to the url of any pull request and you get a single file that describes the whole PR.

YannickJadoul · 2020-12-29T17:49:27Z

@Czaki, so I'm pushing?

Czaki · 2020-12-29T18:12:10Z

Push. I do not know why I have a problem with patch the whole directory (so I split on two files).

… patch

YannickJadoul · 2020-12-29T18:27:35Z

I sneaked in another commit, removing the pypy_patch function, since it has become so small, and isn't really very informative to read (to me, at least; it doesn't say what patch or anything).

Feel free to discuss/remove, if necessary.

I also added a small test to test_testing.py:

    def test_time_to_remove_the_pypy_venv_patch(self):
        assert not hasattr(sys, "pypy_version_info") or sys.pypy_version_info < (7,3,4)

Not sure if this is a great plan, but it could work as this reminder? (We can't just easily test that in cibuildwheel, because we're not running on PyPy, there; at least, we don't know what we're running on, so probably CPython.)

EDIT: Please review/discuss this hack; I'm not convinced about it myself ;-)

joerick · 2020-12-29T18:33:32Z

I also added a small test to test_testing.py:

Good idea! But I think we were planning to leave the patch in for a couple releases, even after our pinned version has upgraded, because users might still choose to specify an older version via a docker image. So it's perhaps not appropriate. Anyway, since we can check sys.pypy_version_info before applying the patch, it's not causing much harm being there for a little while.

YannickJadoul · 2020-12-29T18:42:51Z

Anyway, since we can check sys.pypy_version_info before applying the patch, it's not causing much harm being there for a little while.

Well, that's the part that's not obvious to do, because you need to check the version of the "guess" PyPy, rather than of the host Python.

Czaki · 2020-12-29T19:35:48Z

Right, but shouldn't we just say that the future version of cibuildwheel that support PyPy 7.3.4 doesn't support PyPy 7.3.3 anymore? If we don't support it on mac anymore, why would we support the docker image for Linux?

There are two questions. If version supporting 7.3.3 should support 7.3.4 (someone updates his docker image without the update of cibuildwheel, because of the rebuild of some dependency). And current implementation supports it.

The second question is what to do when changing the base version of PyPy to 7.3.4. I personally prefer to not break compatibility immediately. Especially in the context of the last Cent Os changes.

YannickJadoul · 2020-12-29T19:50:00Z

There are two questions. If version supporting 7.3.3 should support 7.3.4 (someone updates his docker image without the update of cibuildwheel, because of the rebuild of some dependency). And current implementation supports it.

That's a good point though; I had only thought about backwards compatibility.

The second question is what to do when changing the base version of PyPy to 7.3.4. I personally prefer to not break compatibility immediately. Especially in the context of the last Cent Os changes.

Well, but then you should stay on an older version of cibuildwheel, I'd say. It's pretty bad practice to build on a different PyPy version on macOS than on Linux, no?
But fair enough.

Czaki · 2020-12-29T20:07:44Z

Well, but then you should stay on an older version of cibuildwheel, I'd say. It's pretty bad practice to build on a different PyPy version on macOS than on Linux, no?
But fair enough.

Maybe a fast drop is a good decision when adding proper information to the documentation.

joerick · 2020-12-29T20:11:00Z

It's pretty bad practice to build on a different PyPy version on macOS than on Linux, no?

Good point, I had not considered this. In that case I'd also be in favour of a quick drop once we have the bug fix in pypy.

YannickJadoul · 2020-12-29T22:45:33Z

@Czaki, I think you're looking for

assert sys.platform != "darwin" or not hasattr(sys, "pypy_version_info") or sys.pypy_version_info < (7,3,4)

if you only want the last two parts tested on macOS.

YannickJadoul · 2020-12-29T22:53:17Z

Something seems to be going horribly wrong, though? Tests just stall after printing test/test_0_basic.py :-(

Czaki · 2020-12-29T22:58:24Z

Something seems to be going horribly wrong, though? Tests just stall after printing test/test_0_basic.py :-(

I suggest wait. Maybe some download fail. I suggest restart tomorrow morning. Last changes does not affect test_0_basic.

henryiii · 2020-12-30T16:20:21Z

All linux builds on all CIs are stalling out still. :(

Czaki · 2020-12-31T00:20:48Z

The bug comes from @YannickJadoul patch.

patch -N -d /opt/python/pp27-pypy_73 -i /pypy_venv.patch 
can't find file to patch at input line 3
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|--- a/lib-python/2.7/sysconfig.py
|+++ b/lib-python/2.7/sysconfig.py
--------------------------
File to patch: 
Skip this patch? [y] n 
File to patch:

Pure call CLI

patch -N -d /opt/python/pp27-pypy_73 /pypy_venv.patch

Block execution waiting on stdinput.

So this is the problem why I split the patch into two files.

It looks like macOS patch works differently than Linux one. Did we revert @YannickJadoul changes?

Czaki · 2020-12-31T00:52:03Z

I have hope that I fix it. I suggest keeping pytest-timeout for better debug.

YannickJadoul · 2020-12-31T01:01:35Z

This is weird. I tried this out, and things passed. What has changed meanwhile?

EDIT: Aha, seems they also timed out; I saw macOS passing and I didn't see Linux failing, so I thought it was OK...

YannickJadoul · 2020-12-31T01:06:44Z

cibuildwheel/linux.py

+                        patch_docker_path = PurePath('/pypy_venv.patch')
+                        docker.copy_into(patch_path, patch_docker_path)
+                        try:
+                            docker.call(['patch', '-N', '-p1', '-d', config.path, '-i', patch_docker_path])


Why -N, why -i?

-N --forward Ignore patches that appear to be reversed or already applied.

I don't see the point of this? We know our patches should apply forward?

-i PATCHFILE --input=PATCHFILE Read patch from PATCHFILE instead of stdin.

This was also not the issue in what you showed. The issue seems to have been the leading a/b?

Better apply one of these, then?

-t --batch Ask no questions; skip bad-Prereq patches; assume reversed.
-f --force Like -t, but ignore bad-Prereq patches, and assume unreversed.

The leading a/b character should be fixed by -p1. The -i (or --input) is correct, we want to specify an input patch, even if it works without it. I think --force is probably what you want here, though, since you never want it to ask questions, which I think -N still could do if it's not one of {unpatched, already patched}.

Suggested change

docker.call(['patch', '-N', '-p1', '-d', config.path, '-i', patch_docker_path])

docker.call(['patch', '--force', '-p1', '-d', config.path, '--input', patch_docker_path])

Perhaps?

-i is needed to specify the patch file. Patch on Linux assumes that the first file is one to patch.

I see no difference in behavior between -N and --force. We have properly formated patch files.

YannickJadoul · 2020-12-31T01:07:23Z

I suggest keeping pytest-timeout for better debug.

That's going to be annoying for the occasional slow test? Why not just add it when debugging?

henryiii · 2020-12-31T01:11:35Z

That's going to be annoying for the occasional slow test? Why not just add it when debugging?

If a test suddenly takes 3x longer, isn't that a bug? As long as the timeout isn't too short so that it triggers on reasonable runs, there's no reason we need to wait 1-6 hours (depending on the CI) to fail?

YannickJadoul · 2020-12-31T01:43:00Z

If a test suddenly takes 3x longer, isn't that a bug? As long as the timeout isn't too short so that it triggers on reasonable runs, there's no reason we need to wait 1-6 hours (depending on the CI) to fail?

Probably. I was just thinking that we now already get the occasional spurious CI failure (just restarted a Travis job), so... myeah

Czaki · 2020-12-31T08:29:17Z

The main difference is that job killed by CI timeout does not provide any information. We do not know in which phase it stuck. Maybe timeout should be increased, but for me, it is better to get a job killed by pytest-timeout than CI timeout. I got more information.

YannickJadoul · 2020-12-31T12:18:41Z

The main difference is that job killed by CI timeout does not provide any information. We do not know in which phase it stuck. Maybe timeout should be increased, but for me, it is better to get a job killed by pytest-timeout than CI timeout. I got more information.

That's true, that's why I suggested to keep it as way of debugging. But fair enough, if the timeout is long enough, it can stay as well

joerick · 2020-12-31T14:47:47Z

I'm happy with keeping the timeout. I'm not particularly familiar with patch, but --force seems more appropriate in our situation? Other than that this looks good 👍

Czaki added 4 commits December 27, 2020 20:14

patching linux and macos

91dcaf5

try test

eb0fcb0

add patch files for pypy2.7

780d8fb

fix installing

d1acc08

Czaki marked this pull request as ready for review December 28, 2020 17:37

henryiii mentioned this pull request Dec 29, 2020

Support qemu in GitHub Actions #482

Merged

YannickJadoul force-pushed the pypy_patch_venv branch from c737c3d to d1acc08 Compare December 29, 2020 16:22

Combine patch files into one

615e905

Simplify further, and add test to remind ourselves to remove the PyPy…

504dda4

… patch

fix test

bbd71eb

Czaki force-pushed the pypy_patch_venv branch from c8b48c8 to bbd71eb Compare December 30, 2020 14:16

henryiii closed this Dec 30, 2020

henryiii reopened this Dec 30, 2020

add timeout

c78c2b0

fix patch on linux

577cde9

YannickJadoul reviewed Dec 31, 2020

View reviewed changes

change -N to --force

3bb6086

joerick approved these changes Dec 31, 2020

View reviewed changes

henryiii merged commit 99203a8 into pypa:master Dec 31, 2020

Czaki deleted the pypy_patch_venv branch December 31, 2020 16:55

joerick mentioned this pull request Jan 1, 2021

PyPy virtualenv headers not available #501

Closed

YannickJadoul mentioned this pull request Jan 1, 2021

Macos patch fix #506

Merged

	docker.call(['patch', '-N', '-p1', '-d', config.path, '-i', patch_docker_path])
	docker.call(['patch', '--force', '-p1', '-d', config.path, '--input', patch_docker_path])

Patching pypy venv #502

Patching pypy venv #502

Conversation

Czaki commented Dec 27, 2020

Czaki commented Dec 28, 2020

mattip commented Dec 28, 2020

henryiii commented Dec 28, 2020

Czaki commented Dec 28, 2020 • edited Loading

henryiii commented Dec 28, 2020

Czaki commented Dec 28, 2020

mattip commented Dec 28, 2020

joerick commented Dec 29, 2020

henryiii commented Dec 29, 2020

Czaki commented Dec 29, 2020

joerick commented Dec 29, 2020

YannickJadoul commented Dec 29, 2020

YannickJadoul commented Dec 29, 2020

Czaki commented Dec 29, 2020

YannickJadoul commented Dec 29, 2020

henryiii commented Dec 29, 2020

YannickJadoul commented Dec 29, 2020

Czaki commented Dec 29, 2020 • edited Loading

YannickJadoul commented Dec 29, 2020 • edited Loading

joerick commented Dec 29, 2020

YannickJadoul commented Dec 29, 2020

Czaki commented Dec 29, 2020

YannickJadoul commented Dec 29, 2020

Czaki commented Dec 29, 2020

joerick commented Dec 29, 2020

YannickJadoul commented Dec 29, 2020

YannickJadoul commented Dec 29, 2020

Czaki commented Dec 29, 2020

henryiii commented Dec 30, 2020

Czaki commented Dec 31, 2020

Czaki commented Dec 31, 2020

YannickJadoul commented Dec 31, 2020 • edited Loading

YannickJadoul Dec 31, 2020

Choose a reason for hiding this comment

henryiii Dec 31, 2020

Choose a reason for hiding this comment

henryiii Dec 31, 2020

Choose a reason for hiding this comment

Czaki Dec 31, 2020

Choose a reason for hiding this comment

YannickJadoul commented Dec 31, 2020

henryiii commented Dec 31, 2020

YannickJadoul commented Dec 31, 2020

Czaki commented Dec 31, 2020

YannickJadoul commented Dec 31, 2020

joerick commented Dec 31, 2020

Czaki commented Dec 28, 2020 •

edited

Loading

Czaki commented Dec 29, 2020 •

edited

Loading

YannickJadoul commented Dec 29, 2020 •

edited

Loading

YannickJadoul commented Dec 31, 2020 •

edited

Loading