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

CMake correctness; streamline nix-shell environment #343

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

bb010g
Copy link

@bb010g bb010g commented Jul 30, 2021

These allow nix-shell --pure --show-trace --keep-going --run 'cargo build --release' from a clean checkout to succeed.

Don't assume LLVM & Clang share the same lib dir

Also, don't assume llvm-config --libdir shares the same lib/ root as llvm-config --cmakedir.

streamline nix-shell environment

Rustup is no longer required when using nix-shell. Instead, the required Rust nightly is already available.

bb010g added 2 commits July 30, 2021 04:08
Also, don't assume `llvm-config --libdir` shares the same `lib/` root as
`llvm-config --cmakedir`.
Rustup is no longer required when using `nix-shell`.
Instead, the required Rust nightly is already available.
@flosse
Copy link

flosse commented Oct 13, 2021

awesome! I did a nix-shell + cargo install --path c2rust --locked and it worked out of the box 🎉

Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

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

This looks good overall and improves error reporting, which is always a plus. I'm not a nix expert but the nix changes look plausible.

Comment on lines 283 to +286
let llvm_config_missing = "
Couldn't find `llvm-config`. Make sure `llvm-config` is on $PATH then
re-build.
";
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, with these changes this string becomes dead. But I'm not opposed to keeping it around as a reminder to add better error handling for this situation.

c2rust-ast-exporter/build.rs Show resolved Hide resolved
@kkysen
Copy link
Contributor

kkysen commented Jun 29, 2022

@bb010g @fw-immunant, can this be merged now? @fw-immunant approved it already and I just fixed the merge conflicts.

@thedataking, the c2rust-testsuite CI is still failing, though, even though c2rust-testsuite is now public. I think that's because the .github/workflows/internal-testsuite.yml still uses a secret token, which it seems is what the CI is failing on. Then there's also the slack fail in the same CI workflow.

@kkysen kkysen added the building Build/compile errors or build system-related label Jun 29, 2022
@kkysen
Copy link
Contributor

kkysen commented Jun 29, 2022

CI is now failing after I re-triggered it with the merge:

CI failure on Ubuntu 18

          Couldn't find Clang cmake dir. Try setting the `CLANG_CMAKE_DIR` environment
          variable or make sure `llvm-config` is on $PATH then re-build. For example:

            $ export CLANG_CMAKE_DIR=/usr/local/opt/llvm/lib/cmake/clang
          : Os { code: 2, kind: NotFound, message: "No such file or directory" }', c2rust-ast-exporter/build.rs:343:22

This might be flaky CI again since it only happened on Ubuntu 18.

@kkysen
Copy link
Contributor

kkysen commented Jun 29, 2022

Ubuntu 18 CI failed again, so it's probably not flaky. I'm not sure why it's only happening in Ubuntu 18, and not even in Ubuntu 20. The build scripts for the docker containers for those should be exactly the same.

…ken in GitHub Actions fixes the error as the repo is now public.
@kkysen
Copy link
Contributor

kkysen commented Jun 29, 2022

c54f7ca, removing the token field, did fix the immunant/c2rust-testsuite checkout issue in GitHub Actions.

@kkysen
Copy link
Contributor

kkysen commented Jun 29, 2022

It's failing in Ubuntu 18 because Ubuntu 18 uses llvm 6, which doesn't have a $(llvm-config --cmake-dir)/../clang directory, and in fact, llvm doesn't have this directory until llvm 9. We'll have to find a fix for that. @bb010g, do llvm and clang need separate directories before llvm 9? The changes were in 289d44a. Should we have a fallback to the old behavior?

@bb010g
Copy link
Author

bb010g commented Aug 17, 2022

@kkysen A fallback sounds reasonable. I don't think I tested on older LLVMs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build/compile errors or build system-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants