-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix segmentation fault when building Python 3.6 image #190
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
Fix segmentation fault when building Python 3.6 image #190
Conversation
@tianon I can totally understand your point about the optimizations. The segmentation fault still remains as an issue though. If you are using the regular build an run the unittest, it basically fails with the same error as described in #160. How do you feel about removing |
3.6/alpine/Dockerfile
Outdated
&& rm -r Modules/expat \ | ||
Modules/zlib \ | ||
Modules/_ctypes/darwin* \ | ||
Modules/_ctypes/libffi* \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a mix of tabs and spaces throughout these changes; can you ensure to only use tabs. 🙏 Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. Sorry about that. I've updated both files with tabs.
Dockerfile-alpine.template
Outdated
&& ./configure \ | ||
--with-system-ffi \ | ||
--with-system-expat \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will these changes affect the other versions of alpine python (3.3, 3.4, and 3.5)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version of Python 3.x seemed to have worked without similar segmentation faults. But I've just run the regression tests in the Python 3.4 container with the settings above and they behaved the same way as without. So it does seem to work either way. I haven't tested all 3 versions, though.
Do you want me to push updated Dockerfiles generated with the update.sh
script and let them run on CI to test?
It would also be possible to use the docker --build-arg
setting for running the unittest as part of the build process locally. For instance, adding this to the Alpine Dockerfiles:
...
ARG RUN_TESTS
...
&& make -j$(getconf _NPROCESSORS_ONLN) \
&& if [ $RUN_TESTS ]; then LD_LIBRARY_PATH=/usr/src/python ./python -m test.regrtest; fi \
&& make install \
...
And then running the docker command with RUN_TESTS=1
would also run the tests without modifying any of the Docker images hosted on the Hub:
docker build --build-arg RUN_TESTS=1 --tag tested-python 3.4/alpine
Would that be something that makes sense to add for testing purposes? What do you suggest as the next steps?
This fixes the issues reported in #160 for the Alpine build. For Python 3.6, the build succeeds but test illustrate that `ctypes` related test fail with segmentation faults. This also prevents optimized builds because they rely on a full run of the test suite. The fix is based on the release candidate build of Python 3.6.1 in Alpine Edge. It switches to system libraries for `libffi` and `expat` in the same way it's used in [the Alpine build file](https://git.alpinelinux.org/cgit/aports/tree/main/python3/APKBUILD).
I'm not 100% sure what the guidelines or rules are around PRs like this, happy to make any adjustments that might be required to get this merged.
This fixes the issues reported in #160 for the Alpine build. For Python
3.6, the build succeeds but test illustrate that
ctypes
related testfail with segmentation faults. This also prevents optimized builds
because they rely on a full run of the test suite.
The fix is based on the release candidate build of Python 3.6.1 in
Alpine Edge. It switches to system libraries for
libffi
andexpat
inthe same way it's used in the Alpine build
file.