-
Notifications
You must be signed in to change notification settings - Fork 8
Added Docker image for older ARMv7 architecture support because of glibc issues #56
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
base: main
Are you sure you want to change the base?
Conversation
👋 Hello jakub-kocka, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
643ce4d to
02bbd49
Compare
02bbd49 to
7c2f199
Compare
peterdragun
left a comment
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.
LGTM, this fixes the issue for us in the esptool. Thank you!
7c2f199 to
bdf9bd8
Compare
dobairoland
left a comment
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.
LGTM.
docker/Dockerfile.manylinux_armv7l
Outdated
| # Build and install patchelf from source to ensure proper ELF alignment on ARM | ||
| # System patchelf versions can cause "ELF load command address/offset not properly aligned" errors | ||
| RUN cd /tmp && \ | ||
| git clone https://github.com/NixOS/patchelf.git && \ |
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.
You could fetch just the branch in order to speed up the process. Full clone is not necessary.
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.
I think we don't really aim for speed in these images. Since it is already pretty slow, the main point is that it produces wheels twice a week without our hosted runners.
Maybe we can leave it as it is for now if you agree.
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.
I approved the PR so all my comments are optional and no need to do another round of review.
But the under the line comment is that maybe that is why this is "already pretty slow" because you reject even the simple-to-implement performance improvements. I don't advocate for unmeaningful optimization. But if something can be done without no significant effort then why not to do it? A full git clone can blow up the size by downloading the full history and with huge amount of git files. It is pretty common to use multi-stage builds in order to copy only the relevant files into the final image for size optimization. I haven't suggested this, only to avoid cloning everything. That is not just about speed of cloning but the size of the image and then speed of copying it elsewhere.
bdf9bd8 to
8d7e2a9
Compare
|
I have removed the Docker files and updated the README.md. Are you OK with this approach, @dobairoland? |
Description
I have dropped Python 3.14 support for the legacy ARMv7 runner because it would bring more implementation issues, and I believe the time period between these old OSes and Python 3.14 is significant enough not to support it.
Related
Internal related tracker: IDFCI-8006
Testing
Action run: https://github.com/espressif/idf-python-wheels/actions/runs/20991228759
Checklist
Before submitting a Pull Request, please ensure the following: