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.

Sign up for GitHub

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

Jump to bottom

Fix nix build #1047

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Fix nix build #1047

merged 1 commit into from
Dec 11, 2023

Conversation

HKalbasi
Copy link
Contributor

@HKalbasi HKalbasi commented Nov 29, 2023 •
edited by kkysen
Loading

@HKalbasi
Copy link
Contributor Author

HKalbasi commented Nov 30, 2023 •
edited
Loading

I think the CI failure is unrelated, but if it is not then please help me to fix it.

@thedataking
Copy link
Contributor

I think the CI failure is unrelated, but if it is not then please help me to fix it.

Yes, one failure is a likely a permissions problem (c2rust-testsuite) which you can ignore. The other was a macOS provisioning problem which is hopefully (🤞) fixed now.

kkysen
kkysen approved these changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

I made a couple minor suggestions, but overall I think this looks fine, though I don't really know much about nix so I can't really review that part of it. I think if this works for you on nix then it's fine, though.

c2rust-ast-exporter/build.rs Outdated
.define("Clang_DIR", &format!("{}/cmake/clang", llvm_lib_dir))
.define(
"LLVM_DIR",
&env::var("CMAKE_LLVM_DIR").unwrap_or(format!("{}/cmake/llvm", llvm_lib_dir)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
&env::var("CMAKE_LLVM_DIR").unwrap_or(format!("{}/cmake/llvm", llvm_lib_dir)),
&env::var("CMAKE_LLVM_DIR").unwrap_or_else(|| format!("{}/cmake/llvm", llvm_lib_dir)),

c2rust-ast-exporter/build.rs Outdated
)
.define(
"Clang_DIR",
&env::var("CMAKE_CLANG_DIR").unwrap_or(format!("{}/cmake/clang", llvm_lib_dir)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
&env::var("CMAKE_CLANG_DIR").unwrap_or(format!("{}/cmake/clang", llvm_lib_dir)),
&env::var("CMAKE_CLANG_DIR").unwrap_or_else(|| format!("{}/cmake/clang", llvm_lib_dir)),

@kkysen kkysen added the building Build/compile errors or build system-related label Dec 1, 2023
@HKalbasi
Copy link
Contributor Author

HKalbasi commented Dec 1, 2023

I don't really know much about nix so I can't really review that part of it. I think if this works for you on nix then it's fine, though

I can add nix build to CI to check that it works and prevent regressions in future, if desired.

@thedataking
Copy link
Contributor

I can add nix build to CI to check that it works and prevent regressions in future, if desired.

As Khyber said on another PR, we do not maintain the nix files for Rust but accept patches when something breaks. Kind of you to offer though; thanks!

@HKalbasi
Copy link
Contributor Author

Anything else to do on this?

@kkysen kkysen merged commit 21907d8 into immunant:master Dec 11, 2023
8 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Sep 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkysen kkysen kkysen approved these changes

Assignees
No one assigned
Labels
building Build/compile errors or build system-related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot build on nix
3 participants
@HKalbasi @thedataking @kkysen