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

Pregeneration of NASM build artifacts #491

Merged
merged 37 commits into from
Aug 27, 2024

Conversation

justsmth
Copy link
Contributor

@justsmth justsmth commented Aug 6, 2024

Issues:

Addresses #364

Description of changes:

  • Provide prebuilt NASM artifacts for build environments that lack a NASM assembler.

Call-outs:

  • Usage of NASM artifacts is an "opt-in". It is determined as follows (subject to further discussion):
    • AWS_LC_SYS_PREBUILT_NASM=1 - Prebuilt artifacts used only if NASM was not found.
    • AWS_LC_SYS_PREBUILT_NASM=0 - NASM is required.
    • Unset - NASM is required.

Testing:

  • Added the "prebuilt NASM verification" step that disassembles newly built NASM objects. It compares their disassembly to the disassembly of the NASM objects already in the repo. The CI job fails if they differ.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@justsmth justsmth changed the title Generate/aws lc sys pregen nasm Pregeneration of NASM build artifacts Aug 6, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.48%. Comparing base (c358484) to head (144c689).
Report is 64 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
- Coverage   95.80%   92.48%   -3.32%     
==========================================
  Files          61       67       +6     
  Lines        8143     9235    +1092     
  Branches        0     9235    +9235     
==========================================
+ Hits         7801     8541     +740     
- Misses        342      422      +80     
- Partials        0      272     +272     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djc
Copy link

djc commented Aug 7, 2024

Have you considered using a Cargo feature instead of an environment variable to make this work? That seems like a solution with better ergonomics for most downstream Rust applications.

@justsmth
Copy link
Contributor Author

justsmth commented Aug 7, 2024

Have you considered using a Cargo feature instead of an environment variable to make this work? That seems like a solution with better ergonomics for most downstream Rust applications.

We considered using a feature for this. The concern is that we want the decision of whether it's acceptable to use the pre-built binaries to be made by the end consumer within their build environment. Because features are only "additive" and features can be enabled by any crate that's part of a build, it seems like the wrong mechanism.

As a feature, the consumer would essentially be required to "delegate" the decision of whether to use NASM pre-built binaries to any/every crate in their dependency tree. One possible compromise might be to expose a feature that specifies what to default to when AWS_LC_SYS_PREBUILT_NASM is not set in the environment -- but this also seems to violate the nature/purpose of "features" as the environment will be "subtracting" a "feature" that a dependent crate indicated that it needed.

We considered both "environment variables" and "features" as mechanisms for enabling this. Is there another option that would also enable intermediate crates (i.e., those part of the dependency tree between us and the end consumer) to influence what the default choice is?

@djc
Copy link

djc commented Aug 8, 2024

We considered both "environment variables" and "features" as mechanisms for enabling this. Is there another option that would also enable intermediate crates (i.e., those part of the dependency tree between us and the end consumer) to influence what the default choice is?

No, I don't think so. Your rationale makes sense to me, unfortunately Cargo doesn't support anything like this right now (see https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618 for some discussion -- and if you think there is appetite for sponsoring work on this at AWS, I'd be interested in discussing that!).

As a downstream application builder I would still prefer being able to control this via features, maybe just with documentation guidance that libraries should not touch this and potentially keeping a way to override it via the environment variable?

justinwsmith and others added 10 commits August 22, 2024 14:37
Update CI

diff --git a/.github/workflows/sys-bindings-generator.yml b/.github/workflows/sys-bindings-generator.yml
index 3fba893d80..dc226f9f1b 100644
--- a/.github/workflows/sys-bindings-generator.yml
+++ b/.github/workflows/sys-bindings-generator.yml
@@ -270,3 +270,36 @@ jobs:
         run: ./scripts/build/collect_build_src.sh -t ${{ matrix.target }}
       - name: Commit & Push changes
         run: ./scripts/ci/ci_add_commit_rebase_push.sh "Collected source files for ${{ matrix.target }}"
+  collect-nasm-and-commit:
+    needs: generate-windows-bindings-and-commit
+    if: github.repository == 'aws/aws-lc-rs'
+    runs-on: windows-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: 'recursive'
+          ref: ${{ github.ref_name }}
+      - uses: dtolnay/rust-toolchain@master
+        id: toolchain
+        with:
+          toolchain: stable
+          targets: "x86_64-pc-windows-msvc,x86_64-pc-windows-gnu"
+      - uses: ilammy/setup-nasm@v1
+      - name: Build aws-lc-sys
+        shell: bash
+        run: AWS_LC_SYS_PREBUILT_NASM=0 cargo build -p aws-lc-sys --release --target x86_64-pc-windows-msvc
+      - name: Collect NASM object files
+        shell: bash
+        run: ./scripts/build/collect_nasm_obj.sh
+      - name: Clean build
+        shell: bash
+        run: cargo clean
+      - name: Test aws-lc-rs for x86_64-pc-windows-msvc
+        shell: bash
+        run: AWS_LC_SYS_PREBUILT_NASM=1 cargo build -p aws-lc-sys --target x86_64-pc-windows-msvc
+      - name: Test aws-lc-sys for x86_64-pc-windows-gnu
+        shell: bash
+        run: AWS_LC_SYS_PREBUILT_NASM=1 cargo build -p aws-lc-sys --target x86_64-pc-windows-gnu
+      - name: Commit & Push changes
+        shell: bash
+        run: ./scripts/ci/ci_add_commit_rebase_push.sh "Collected NASM files"
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
index a0da3f48b8..71716a64c0 100644
--- a/.github/workflows/tests.yml
+++ b/.github/workflows/tests.yml
@@ -37,7 +37,7 @@ jobs:
           - --no-default-features --features non-fips,ring-sig-verify,unstable
           - --no-default-features --features non-fips,alloc,unstable
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
       - uses: dtolnay/rust-toolchain@master
@@ -66,7 +66,7 @@ jobs:
           - --no-default-features --features aws-lc-sys,bindgen,unstable
           - --release --all-targets --features bindgen,unstable
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
       - if: ${{ matrix.os == 'macos-13-xlarge' }}
@@ -106,9 +106,10 @@ jobs:
           - --no-default-features --features non-fips,ring-io,unstable
           - --no-default-features --features non-fips,ring-sig-verify,unstable
           - --no-default-features --features non-fips,alloc,unstable
+    env:
+      AWS_LC_SYS_PREBUILT_NASM: 1
     steps:
-      - uses: ilammy/setup-nasm@v1
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
       - uses: dtolnay/rust-toolchain@master
@@ -125,7 +126,7 @@ jobs:
     name: aws-ls-rs coverage
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
           lfs: true
@@ -167,7 +168,7 @@ jobs:
           - --no-default-features --features fips,asan
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
       - uses: dtolnay/rust-toolchain@master
@@ -196,7 +197,7 @@ jobs:
         os: [ ubuntu-latest, macos-12, macos-13-xlarge ]
         static: [ 0, 1 ]
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
       - uses: dtolnay/rust-toolchain@stable
@@ -205,6 +206,40 @@ jobs:
         # See: rust-lang/cargo#8531
         run: cargo test -p aws-lc-rs --tests

+  build-env-nasm-test:
+    if: github.repository_owner == 'aws'
+    name: prebuilt NASM verification
+    runs-on: windows-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        target:
+          - 'x86_64-pc-windows-msvc'
+          - 'x86_64-pc-windows-gnu'
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: 'recursive'
+      - uses: dtolnay/rust-toolchain@stable
+      - name: Install NASM
+        uses: ilammy/setup-nasm@v1
+      - name: Remove NASM artifacts
+        shell: bash
+        run: |
+          cargo clean
+          rm ./aws-lc-sys/builder/prebuilt-nasm/*
+      - name: Run cargo test
+        shell: bash
+        run: AWS_LC_SYS_PREBUILT_NASM=0 cargo test --tests -p aws-lc-rs --release --no-default-features --features aws-lc-sys
+      - name: Collect NASM outputs
+        shell: bash
+        run: ./scripts/build/collect_nasm_obj.sh
+      - name: Flag any NASM changes
+        shell: bash
+        run: |
+          git add .
+          git diff --cached --exit-code HEAD -- aws-lc-sys/builder/prebuilt-nasm/*.txt
+
   build-env-external-bindgen-test:
     if: github.repository_owner == 'aws'
     name: aws-lc-rs FIPS - External bindgen test
@@ -216,14 +251,14 @@ jobs:
       matrix:
         os: [ ubuntu-latest, macos-12, macos-13-xlarge, windows-latest ]
     steps:
-      - if: ${{ matrix.os == 'windows-latest' }}
-        uses: ilammy/setup-nasm@v1
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
       - uses: dtolnay/rust-toolchain@stable
-      - name: Install ninja-build tool
-        uses: seanmiddleditch/gha-setup-ninja@v4
+      - if: ${{ matrix.os == 'windows-latest' }}
+        uses: ilammy/setup-nasm@v1
+      - if: ${{ matrix.os == 'windows-latest' }}
+        uses: seanmiddleditch/gha-setup-ninja@v5
       - name: Install bindgen-cli
         run: cargo install --locked bindgen-cli
       - name: Remove bindings
@@ -245,12 +280,10 @@ jobs:
         os: [ ubuntu-latest, macos-12, macos-13-xlarge ]
         static: [ 0, 1 ]
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
       - uses: dtolnay/rust-toolchain@stable
-      - name: Install ninja-build tool
-        uses: seanmiddleditch/gha-setup-ninja@v4
       - uses: actions/setup-go@v4
         with:
           go-version: '>=1.18'
@@ -271,7 +304,7 @@ jobs:
       matrix:
         os: [ ubuntu-latest, macos-12, macos-13-xlarge, windows-latest ]
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
       - uses: dtolnay/rust-toolchain@stable
@@ -306,15 +339,14 @@ jobs:
       matrix:
         os: [ ubuntu-latest, macos-12, macos-13-xlarge, windows-latest ]
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
       - uses: dtolnay/rust-toolchain@stable
-      - name: Install ninja-build tool
-        uses: seanmiddleditch/gha-setup-ninja@v4
       - uses: actions/setup-go@v4
         with:
           go-version: '>=1.18'
+      - uses: seanmiddleditch/gha-setup-ninja@v5
       - name: Run cargo test
         run: cargo test -p aws-lc-rs --tests --no-default-features --features fips
       - name: Release build
@@ -346,14 +378,14 @@ jobs:
       matrix:
         os: [ ubuntu-latest, macos-12, macos-13-xlarge, windows-latest ]
     steps:
-      - if: ${{ matrix.os == 'windows-latest' }}
-        uses: ilammy/setup-nasm@v1
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
+      - if: ${{ matrix.os == 'windows-latest' }}
+        uses: ilammy/setup-nasm@v1
+      - if: ${{ matrix.os == 'windows-latest' }}
+        uses: seanmiddleditch/gha-setup-ninja@v5
       - uses: dtolnay/rust-toolchain@stable
-      - name: Install ninja-build tool
-        uses: seanmiddleditch/gha-setup-ninja@v4
       - uses: actions/setup-go@v4
         with:
           go-version: '>=1.18'
@@ -378,7 +410,7 @@ jobs:
           - macos-12
           - macos-13-xlarge
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
         with:
           submodules: 'recursive'
           lfs: true
diff --git a/aws-lc-sys/builder/cmake_builder.rs b/aws-lc-sys/builder/cmake_builder.rs
index e25cecb443..c5069c5341 100644
--- a/aws-lc-sys/builder/cmake_builder.rs
+++ b/aws-lc-sys/builder/cmake_builder.rs
@@ -3,9 +3,9 @@

 use crate::OutputLib::{Crypto, RustWrapper, Ssl};
 use crate::{
-    cargo_env, emit_warning, execute_command, is_crt_static, is_no_asm, option_env, target,
-    target_arch, target_env, target_family, target_os, target_underscored, target_vendor,
-    OutputLibType,
+    allow_prebuilt_nasm, cargo_env, emit_warning, execute_command, is_crt_static, is_no_asm,
+    option_env, target, target_arch, target_env, target_family, target_os, target_underscored,
+    target_vendor, test_nasm_command, OutputLibType,
 };
 use std::env;
 use std::ffi::OsString;
@@ -22,10 +22,6 @@ fn test_clang_cl_command() -> bool {
     execute_command("clang-cl".as_ref(), &["--version".as_ref()]).status
 }

-fn test_nasm_command() -> bool {
-    execute_command("nasm".as_ref(), &["-version".as_ref()]).status
-}
-
 fn find_cmake_command() -> Option<OsString> {
     if let Some(cmake) = option_env("CMAKE") {
         emit_warning(&format!(
@@ -162,7 +158,7 @@ impl CmakeBuilder {

         // See issue: aws#453
         if target_os() == "windows" {
-            Self::configure_windows(&mut cmake_cfg);
+            self.configure_windows(&mut cmake_cfg);
         }

         // If the build environment vendor is Apple
@@ -213,7 +209,7 @@ impl CmakeBuilder {
         cmake_cfg
     }

-    fn configure_windows(cmake_cfg: &mut cmake::Config) {
+    fn configure_windows(&self, cmake_cfg: &mut cmake::Config) {
         match (target_env().as_str(), target_arch().as_str()) {
             ("msvc", "aarch64") => {
                 cmake_cfg.generator_toolset(format!(
@@ -243,6 +239,22 @@ impl CmakeBuilder {
             }
             _ => {}
         }
+        if target_arch() == "x86_64" && Some(true) == allow_prebuilt_nasm() {
+            emit_warning("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");
+            emit_warning("!!!   Using pre-built NASM binaries   !!!");
+            emit_warning("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");
+
+            let script_path = self
+                .manifest_dir
+                .join("builder")
+                .join("prebuilt-nasm.bat")
+                .display()
+                .to_string();
+            let script_path = script_path.replace('\\', "/");
+
+            cmake_cfg.define("CMAKE_ASM_NASM_COMPILER", script_path.as_str());
+            cmake_cfg.define("CMAKE_VERBOSE_MAKEFILE", "1");
+        }
     }

     fn configure_open_harmony(cmake_cfg: &mut cmake::Config) {
@@ -298,10 +310,14 @@ impl crate::Builder for CmakeBuilder {
         let mut missing_dependency = false;

         if target_os() == "windows" {
-            if target_arch() == "x86_64" && !test_nasm_command() && !is_no_asm() {
+            if target_arch() == "x86_64"
+                && !is_no_asm()
+                && !test_nasm_command()
+                && Some(true) != allow_prebuilt_nasm()
+            {
                 eprintln!(
-                    "Consider setting `AWS_LC_SYS_NO_ASM` in the environment for development builds.\
-                See User Guide about the limitations: https://aws.github.io/aws-lc-rs/index.html"
+                    "Consider setting `AWS_LC_SYS_PREBUILT_NASM` in the build environment.\
+                See User Guide: https://aws.github.io/aws-lc-rs/index.html"
                 );
                 eprintln!("Missing dependency: nasm");
                 missing_dependency = true;
diff --git a/aws-lc-sys/builder/main.rs b/aws-lc-sys/builder/main.rs
index 23f0214991..987e1038ca 100644
--- a/aws-lc-sys/builder/main.rs
+++ b/aws-lc-sys/builder/main.rs
@@ -319,6 +319,7 @@ static mut AWS_LC_SYS_NO_PREFIX: bool = false;
 static mut AWS_LC_SYS_INTERNAL_BINDGEN: bool = false;
 static mut AWS_LC_SYS_EXTERNAL_BINDGEN: bool = false;
 static mut AWS_LC_SYS_NO_ASM: bool = false;
+static mut AWS_LC_SYS_PREBUILT_NASM: Option<bool> = None;

 fn initialize() {
     unsafe {
@@ -328,6 +329,7 @@ fn initialize() {
         AWS_LC_SYS_EXTERNAL_BINDGEN =
             env_var_to_bool("AWS_LC_SYS_EXTERNAL_BINDGEN").unwrap_or(false);
         AWS_LC_SYS_NO_ASM = env_var_to_bool("AWS_LC_SYS_NO_ASM").unwrap_or(false);
+        AWS_LC_SYS_PREBUILT_NASM = env_var_to_bool("AWS_LC_SYS_PREBUILT_NASM");
     }

     if !is_external_bindgen() && (is_internal_bindgen() || !has_bindgen_feature()) {
@@ -363,6 +365,7 @@ fn is_bindgen_required() -> bool {
         || !has_pregenerated()
 }

+#[allow(dead_code)]
 fn internal_bindgen_supported() -> bool {
     // TODO: internal bindgen creates invalid bindings on FreeBSD
     // See: aws#476
@@ -385,6 +388,10 @@ fn is_no_asm() -> bool {
     unsafe { AWS_LC_SYS_NO_ASM }
 }

+fn allow_prebuilt_nasm() -> Option<bool> {
+    unsafe { AWS_LC_SYS_PREBUILT_NASM }
+}
+
 fn has_bindgen_feature() -> bool {
     cfg!(feature = "bindgen")
 }
@@ -393,6 +400,10 @@ fn has_pregenerated() -> bool {
     unsafe { PREGENERATED }
 }

+fn test_nasm_command() -> bool {
+    execute_command("nasm".as_ref(), &["-version".as_ref()]).status
+}
+
 fn prepare_cargo_cfg() {
     // This is supported in Rust >= 1.77.0
     // Also remove `#![allow(unexpected_cfgs)]` from src/lib.rs
diff --git a/aws-lc-sys/builder/prebuilt-nasm.bat b/aws-lc-sys/builder/prebuilt-nasm.bat
new file mode 100644
index 0000000000..9c761db12a
--- /dev/null
+++ b/aws-lc-sys/builder/prebuilt-nasm.bat
@@ -0,0 +1,21 @@
+@echo off
+set "ScriptDir=%~dp0"
+set "ScriptDir=%ScriptDir:~0,-1%"
+:loop
+set "arg1=%~1"
+if "%arg1%"=="-o" goto end
+if "%arg1%"=="" goto failure
+shift
+goto loop
+:end
+shift
+set "path=%~1"
+for %%f in ("%path%") do set "filename=%%~nxf"
+copy "%ScriptDir%\prebuilt-nasm\%filename%" "%path%"
+exit 0
+
+:failure
+echo PATH: %path% 1>&2
+echo FILENAME: %filename% 1>&2
+echo ScriptDir: %ScriptDir% 1>&2
+exit 1
\ No newline at end of file
diff --git a/scripts/build/collect_nasm_obj.sh b/scripts/build/collect_nasm_obj.sh
new file mode 100644
index 0000000000..c425e9236f
--- /dev/null
+++ b/scripts/build/collect_nasm_obj.sh
@@ -0,0 +1,29 @@
+#!/usr/bin/env bash
+# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+# SPDX-License-Identifier: Apache-2.0 OR ISC
+
+set -ex
+set -o pipefail
+
+if [[ ${BASH_VERSINFO[0]} -lt 4 ]]; then
+    echo Must use bash 4 or later: ${BASH_VERSION}
+    exit 1
+fi
+
+SCRIPT_DIR=$(cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd)
+REPO_ROOT=$(git rev-parse --show-toplevel)
+SYS_CRATE_DIR="${REPO_ROOT}/aws-lc-sys"
+PREBUILT_NASM_DIR="${SYS_CRATE_DIR}/builder/prebuilt-nasm"
+mkdir -p "${PREBUILT_NASM_DIR}"
+rm -f "${PREBUILT_NASM_DIR}"/*
+
+DUMPBIN="$(find /c/Program\ Files/Microsoft\ Visual\ Studio/ -path "*/Hostx64/x64/*" -name "dumpbin.exe" -print -quit)"
+
+for nasm_file in `find aws-lc-sys/aws-lc/generated-src/win-x86_64/ -name "*.asm"`; do
+  OBJNAME=$(basename "${nasm_file}");
+  NASM_OBJ=$(find target/ -name "${OBJNAME/.asm/.obj}");
+  cp "${NASM_OBJ}" "${PREBUILT_NASM_DIR}"
+  # We remove the '.debug$S' value, which indicates the size of the debug section. This value can change across builds
+  # because it typically contains full source file paths that vary by build environment
+  "${DUMPBIN}" //DISASM "${PREBUILT_NASM_DIR}"/"${OBJNAME/.asm/.obj}" | grep -v '.debug$S' | sed -e "s/^Dump of file.*/Dump of file ${OBJNAME/.asm/.obj}/" > "${PREBUILT_NASM_DIR}"/"${OBJNAME/.asm/}"-disasm.txt
+done
@justsmth justsmth force-pushed the generate/aws-lc-sys-pregen-nasm branch from e0786b6 to 69f10f0 Compare August 22, 2024 18:38
@justsmth justsmth marked this pull request as ready for review August 22, 2024 18:38
@justsmth justsmth requested a review from a team as a code owner August 22, 2024 18:38
@justsmth
Copy link
Contributor Author

@djc -- Thanks for your feedback!

We are still discussing this issue internally. We may follow up in a subsequent PR to adjust the build environment conditions that determine whether the pre-built NASM objects are allowed to be used.

.github/workflows/tests.yml Show resolved Hide resolved
aws-lc-sys/builder/cmake_builder.rs Show resolved Hide resolved
aws-lc-sys/builder/prebuilt-nasm.bat Show resolved Hide resolved
@justsmth justsmth merged commit f4e09c2 into aws:main Aug 27, 2024
190 of 198 checks passed
@justsmth justsmth deleted the generate/aws-lc-sys-pregen-nasm branch August 27, 2024 20:05
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.

6 participants