Skip to content
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 docker armv7 cryptography module build #2436

Merged
merged 3 commits into from
Nov 27, 2024
Merged

Conversation

foxfire52
Copy link
Contributor

@foxfire52 foxfire52 commented Nov 27, 2024

#2349
This should fix cryptography module build error.
We can't clean up rust with rustup self uninstall -y because there's a bug Value too large link, but we can remove .rustup folder.

Copy link

Review by g4f copilot

Thank you for contributing to the project!

I've reviewed the pull request, and here are my thoughts:

Summary

The pull request aims to fix the cryptography module build error in the Dockerfile-armv7. The changes include installing additional dependencies, setting up the Rust toolchain, and cleaning up the build environment.

Code Review

The changes look good, and I've verified that the diff addresses the issue mentioned in the description. Here are some specific comments:

  • The addition of curl and libssl-dev as dependencies is reasonable, given the requirements of the cryptography module.
  • The installation of the Rust toolchain using rustup is a good approach, considering the limitations of apt-get for Rust packages.
  • The removal of the .rustup folder is a good workaround for the "Value too large" bug in rustup self uninstall.

Suggestions

  • Consider adding a comment above the RUN curl https://sh.rustup.rs... line to explain the purpose of installing the Rust toolchain.
  • In the ENV PATH line, it's not necessary to include ${HOME}/.local/bin, as it's already included in the default PATH environment variable.

Conclusion

Overall, the changes look good, and I'm happy to see the cryptography module build error fixed. With the suggested minor improvements, this pull request is ready to be merged.

Approved

@hlohaus
Copy link
Collaborator

hlohaus commented Nov 27, 2024

g4f Copilot says: ready to be merged

Thank you

@hlohaus hlohaus merged commit 0cf0c1f into xtekky:main Nov 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants