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

Remove NASM requirement for windows builds #1477

Open
camshaft opened this issue Nov 17, 2023 · 3 comments · Fixed by aws/s2n-quic#2037
Open

Remove NASM requirement for windows builds #1477

camshaft opened this issue Nov 17, 2023 · 3 comments · Fixed by aws/s2n-quic#2037
Labels
enhancement New feature or request

Comments

@camshaft
Copy link

Problem:

When attempting to switch s2n-quic to use aws-lc-rs on all platforms, I ran into an issue for windows where NASM is required to be installed in the environment (see ci build).

While I see the aws-lc-rs CI has worked around this issue by installing NASM, this is an additional dependency that ring doesn't require, and could be considered an additional hurdle to aws-lc-rs being a drop-in ring replacement. Looking at the ring build script, it looks like they've pre-generated files to avoid the NASM dependency. I think it would be best to do the same in aws-lc-rs.

Build output

  cargo:rustc-cfg=use_bindgen_generated
  CMAKE_TOOLCHAIN_FILE_x86_64-pc-windows-msvc = None
  CMAKE_TOOLCHAIN_FILE_x86_64_pc_windows_msvc = None
  HOST_CMAKE_TOOLCHAIN_FILE = None
  CMAKE_TOOLCHAIN_FILE = None
  CMAKE_GENERATOR_x86_64-pc-windows-msvc = None
  CMAKE_GENERATOR_x86_64_pc_windows_msvc = None
  HOST_CMAKE_GENERATOR = None
  CMAKE_GENERATOR = None
  CMAKE_PREFIX_PATH_x86_64-pc-windows-msvc = None
  CMAKE_PREFIX_PATH_x86_64_pc_windows_msvc = None
  HOST_CMAKE_PREFIX_PATH = None
  CMAKE_PREFIX_PATH = None
  CMAKE_x86_64-pc-windows-msvc = None
  CMAKE_x86_64_pc_windows_msvc = None
  HOST_CMAKE = None
  CMAKE = Some("cmake")
  running: "cmake" "C:\\Users\\runneradmin\\.cargo\\registry\\src\\index.crates.io-6f17d22bba15001f\\aws-lc-sys-0.12.0" "-G" "Visual Studio 17 2022" "-Thost=x64" "-Ax64" "-DBUILD_SHARED_LIBS=0" "-DBORINGSSL_PREFIX=aws_lc_0_12_0_" "-DBORINGSSL_PREFIX_HEADERS=C:\\Users\\runneradmin\\.cargo\\registry\\src\\index.crates.io-6f17d22bba15001f\\aws-lc-sys-0.12.0\\generated-include" "-DBUILD_TESTING=OFF" "-DBUILD_LIBSSL=OFF" "-DDISABLE_PERL=ON" "-DDISABLE_GO=ON" "-DCMAKE_INSTALL_PREFIX=D:\\a\\s2n-quic\\s2n-quic\\target\\debug\\build\\aws-lc-sys-47a8a1f3d5620838\\out" "-DCMAKE_C_FLAGS= -nologo -MD -Brepro" "-DCMAKE_C_FLAGS_DEBUG= -nologo -MD -Brepro" "-DCMAKE_CXX_FLAGS= -nologo -MD -Brepro" "-DCMAKE_CXX_FLAGS_DEBUG= -nologo -MD -Brepro" "-DCMAKE_ASM_FLAGS= -nologo -MD -Brepro" "-DCMAKE_ASM_FLAGS_DEBUG= -nologo -MD -Brepro" "-DCMAKE_BUILD_TYPE=Debug" "--no-warn-unused-cli"
  Not searching for unused variables given on the command line.
  -- The C compiler identification is MSVC 19.37.32825.0
  -- Detecting C compiler ABI info
  -- Detecting C compiler ABI info - done
  -- Check for working C compiler: C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.37.32822/bin/Hostx64/x64/cl.exe - skipped
  -- Detecting C compile features
  -- Detecting C compile features - done
  -- Go not found. Disabling some code generation and using pre-generated code in generated-src/
  -- Perl not found. Disabling some code generation and using pre-generated code in generated-src/
  -- The ASM_NASM compiler identification is unknown
  -- Didn't find assembler
  -- Configuring incomplete, errors occurred!

  --- stderr
  CMake Deprecation Warning at CMakeLists.txt:6 (cmake_minimum_required):
    Compatibility with CMake < 3.5 will be removed from a future version of
    CMake.

    Update the VERSION argument <min> value or use a ...<max> suffix to tell
    CMake that the project does not need compatibility with older versions.


  CMake Deprecation Warning at aws-lc/CMakeLists.txt:1 (cmake_minimum_required):
    Compatibility with CMake < 3.5 will be removed from a future version of
    CMake.

    Update the VERSION argument <min> value or use a ...<max> suffix to tell
    CMake that the project does not need compatibility with older versions.


  CMake Error at aws-lc/crypto/CMakeLists.txt:72 (enable_language):
    No CMAKE_ASM_NASM_COMPILER could be found.



  thread 'main' panicked at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cmake-0.1.50\src\lib.rs:1098:5:

  command did not execute successfully, got: exit code: 1

  build script failed, must exit now
  stack backtrace:
     0: std::panicking::begin_panic_handler
               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library\std\src\panicking.rs:597
     1: core::panicking::panic_fmt
               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library\core\src\panicking.rs:72
     2: cmake::find_exe::{{closure}}
     3: <cmake::Version as core::default::Default>::default
     4: cmake::Config::build
     5: build_script_main::prepare_cmake_build::{{closure}}
     6: build_script_main::prepare_cmake_build::{{closure}}
     7: core::ops::function::FnOnce::call_once
     8: <core::result::Result<T,F> as core::ops::try_trait::FromResidual<core::result::Result<core::convert::Infallible,E>>>::from_residual
     9: std::rt::lang_start
    10: std::rt::lang_start_internal::closure$2
               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library\std\src\rt.rs:148
    11: std::panicking::try::do_call
               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library\std\src\panicking.rs:504
    12: std::panicking::try
               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library\std\src\panicking.rs:468
    13: std::panic::catch_unwind
               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library\std\src\panic.rs:142
    14: std::rt::lang_start_internal
               at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library\std\src\rt.rs:148
    15: std::rt::lang_start
    16: main
    17: invoke_main
               at D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78
    18: __scrt_common_main_seh
               at D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    19: BaseThreadInitThunk
    20: RtlUserThreadStart
  note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
warning: build failed, waiting for other jobs to finish...
@skmcgrail
Copy link
Member

skmcgrail commented Nov 20, 2023

This would require support in AW-LC to do so, and should probably have an issue cut there to track it. The issue at hand is that AWS-LC can't do this auto-compilation of NASM source to object files, as then the prefixing of symbols wouldn't work. But we would need some way to override the object files for the windows assembly when it attempts to use the pre-generated sources.

@surban
Copy link

surban commented Apr 30, 2024

Please do not ship any pre-generated code or object files. In the age of supply chain attacks this poses an unnecessary risk and makes verification of dependencies significantly more complex.

If you decide to do so anyway, please make it opt-in only, i.e. by requiring an environment variable (USE_PREGENERATED_OBJ) to be set during build.

@opensource-inemar-net
Copy link

A fundamental library like tls should be batteries included.
That means it should compile on a windows machine, where RUST has been installed using rustup and the recommended procedure.

If this means including pre-build binaries for certain platforms this is the trade-off, which needs to be accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants