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

feat(static): build openssl for fully static corpora in another arm64 linux container #60

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

skyl
Copy link
Owner

@skyl skyl commented Nov 20, 2024

PR Type

enhancement, configuration changes


Description

  • Removed the trailing slash from the 'bin' path in the URL pattern in urls.py to ensure consistency in URL handling.
  • Updated Dockerfile.xcompile to build OpenSSL statically for musl, reorganized the installation of tools, and added environment variables for static linking.
  • Added musl target for Rust in the Dockerfile to support static builds.

Changes walkthrough 📝

Relevant files
Enhancement
urls.py
Remove trailing slash from URL pattern in debug mode         

py/packages/corpora_proj/urls.py

  • Removed trailing slash from the 'bin' path in the URL pattern.
+1/-1     
Configuration changes
Dockerfile.xcompile
Update Dockerfile for static OpenSSL and musl target         

docker/Dockerfile.xcompile

  • Reorganized Dockerfile for building static binaries.
  • Added static OpenSSL build for musl.
  • Updated environment variables for static linking.
  • Added musl target for Rust.
  • +30/-14 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    URL Consistency
    The trailing slash was removed from the 'bin' path in the URL pattern. Ensure this change does not affect any existing functionality or routing logic.

    Build Configuration
    The Dockerfile has been updated to build OpenSSL statically for musl. Verify that the new build configuration works as expected and does not introduce any issues.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Verify the OpenSSL installation paths to prevent build issues

    Ensure that the OpenSSL installation path is correctly set by verifying that the
    make install_sw command installs files in the expected directories, as incorrect
    paths can lead to build failures.

    docker/Dockerfile.xcompile [21]

    -&& make install_sw
    +&& make install_sw && ls /usr/local/lib && ls /usr/local/include
    Suggestion importance[1-10]: 7

    Why: The suggestion to verify the OpenSSL installation paths is valuable as it helps ensure that the installation is successful and files are placed in the expected directories, which can prevent potential build failures.

    7
    Verify Rust installation and musl target addition to prevent runtime errors

    Add a verification step to ensure that the Rust installation and the addition of the
    musl target are successful, as failures in these steps can lead to runtime errors.

    docker/Dockerfile.xcompile [25]

    -RUN curl https://sh.rustup.rs -sSf | sh -s -- -y
    +RUN curl https://sh.rustup.rs -sSf | sh -s -- -y && rustc --version && rustup target list --installed
    Suggestion importance[1-10]: 7

    Why: Adding verification steps for Rust installation and musl target addition is beneficial as it ensures these critical components are correctly set up, reducing the risk of runtime errors due to incomplete installations.

    7

    @skyl skyl merged commit 1010c6a into main Nov 20, 2024
    2 checks passed
    @skyl skyl deleted the static-static branch November 20, 2024 23:48
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant