-
Notifications
You must be signed in to change notification settings - Fork 243
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
Update paths to homebrew LLVM for M1 macs #711
Conversation
Is this the M1 path? Maybe we should just list both paths, and in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/opt/homebrew/
is used by hombrew
on M1 macs, but /usr/local/
is still used on Intel macs, and we want to support both. So we should list both paths in the README.md
, and potentially also say one is for Intel macs and one is for M1 macs (M2, etc., too).
These paths are also hardcoded in c2rust-build-paths/src/lib.rs
and c2rust-analyze/tests/filecheck.rs
. The latter is meant to be consolidated under c2rust-build-paths
. We need to also update these to include the M1 homebrew
paths.
On my system homebrew installed LLVM into `/opt/homebrew/opt/llvm/bin/llvm-config`. After using this updated path, `c2rust` master built successfully. I believe my homebrew is stock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll probably want to eventually generate all these different but similar paths to search, or search by glob/regex or something, but for now, this looks great and updates things in all the right places I believe.
By the way, @fw-immunant, I think we should merge this before publishing a new |
I agree. This looks good to me, but I'm not familiar with Homebrew/macOS. Should we wait for @thedataking to review, or just go ahead and merge? |
@thedataking doesn't have an M1, right? We can wait for him to take a quick look, but I'm not sure he'll have any extra insight. I think if it works on both on @LegNeato's M1 mac and @thedataking's Intel mac it should be good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Note that the CI workflow already tests Intel Macs.
nit: since this patch also fixes capitalization, it should use ARM, not Arm, since it is an acronym. We could also just say Intel Macs and Apple Silicon Macs. No big deal though.
/opt/homebrew/
is used byhombrew
on Apple Silicon Macs, but/usr/local/
is still used on Intel Macs, and we want to support both. This lists both paths in theREADME.md
, clarifying which is for Intel vs. Apple Silicon Macs.These paths are also hardcoded in
c2rust-build-paths/src/lib.rs
andc2rust-analyze/tests/filecheck.rs
. The latter is meant to be consolidated underc2rust-build-paths
, but we'll keep them as is for now. We need to also update these to include the Apple Siliconhomebrew
paths.Now,
c2rust
builds fine on Intel Macs (CI) and Apple Silicon Macs (@LegNeato's M1).