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 Compiling from nargo on M2 Mac #1

Merged
merged 2 commits into from
Oct 14, 2022
Merged

Fix Compiling from nargo on M2 Mac #1

merged 2 commits into from
Oct 14, 2022

Conversation

Savio-Sou
Copy link

@Savio-Sou Savio-Sou commented Oct 11, 2022

This fixes the errors encountered when cargo installing nargo on M2 (which goes through build.rs).

Assume libomp installed with brew.

Assume `libomp` installed with `brew`.
if let OS::Linux = select_os() {
let llvm_dir = find_llvm_linux_path();
println!("cargo:rustc-link-search={}/lib", llvm_dir);
} else if let ARM_APPLE = select_toolchain() {
Copy link

@jfecher jfecher Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're just checking string equality we don't need to use pattern matching here. Should be else if ARM_APPLE == select_toolchain() or the reverse select_toolchain() == ARM_APPLE

Edit: Looks like you wrote it this way because of the OS::Linux / ARM_LINUX lines above/below, I'd prefer if both were updated

Copy link
Author

@Savio-Sou Savio-Sou Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Rewrote matching in the new commit though to include Homebrew path for Intel Macs.

- Add Intel Mac Homebrew path
- Optimise toolchain equality checks / matching
- Reformat comments
let llvm_dir = find_llvm_linux_path();
println!("cargo:rustc-link-search={}/lib", llvm_dir)
}
INTEL_APPLE => println!("cargo:rustc-link-search=/usr/local/lib"),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add Intel Mac Homebrew path

// Link lib OMP
link_lib_omp();
// Link lib OpenMP
link_lib_omp(toolchain);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass retrieved toolchain down to minimise runs of select_toolchain().

@Savio-Sou Savio-Sou changed the title Fix Compiling on M1/M2 Macs Fix Compiling from Noir on M2 Mac Oct 14, 2022
@Savio-Sou Savio-Sou changed the title Fix Compiling from Noir on M2 Mac Fix Compiling from nargo on M2 Mac Oct 14, 2022
@@ -33,14 +31,14 @@ fn select_arch() -> Arch {
let arch = std::env::consts::ARCH;
match arch {
"arm" => Arch::Arm,
"aarch64" => Arch::Arm,
Copy link
Author

@Savio-Sou Savio-Sou Oct 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helps M2 Macs with returned architecture of aarch64 (instead of arm) to pick the correct toolchain.

It prevents them running on the fallback x86_64 config that brings compilation involving assembly code, which would lead to the unknown token in expression error when ran on ARM (described in more details here).

println!("cargo:rustc-link-search={}/lib", llvm_dir)
}
INTEL_APPLE => println!("cargo:rustc-link-search=/usr/local/lib"),
ARM_APPLE => println!("cargo:rustc-link-search=/opt/homebrew/lib"),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the error from clang on macOS failing to find lomp when no search path is provided:

error: linking with `cc` failed: exit status: 1
  = note: ld: library not found for -lomp
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

@kevaundray kevaundray merged commit 582b34e into noir-lang:kw/noir-dsl Oct 14, 2022
@Savio-Sou Savio-Sou deleted the patch-1 branch October 14, 2022 16:10
phated pushed a commit that referenced this pull request Jan 30, 2023
Cmake changes/fixes and some nix changes
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.

3 participants