-
Notifications
You must be signed in to change notification settings - Fork 272
Fix nix build for c2rust #1341
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
base: master
Are you sure you want to change the base?
Fix nix build for c2rust #1341
Conversation
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.
Thanks for the improvements here! To reiterate what I've told some other people, we don't have the resources to maintain and review any of the Nix-related code here, but we're happy to accept fixes. I've reviewed the parts of this that touch the main c2rust files (build.rs
and test_translator.py
), but I haven't looked at the nix-specific files.
Also, I'm not sure if you've seen @RossSmyth's #1285, so I just wanted to make sure the two of you are aware of each other's nix changes and can coordinate anything necessary.
@kkysen thanks for the review! I have pushed the fixes for most of the things you brought up, and I left a couple of them unresolved in order to get your opinion. The main one is the use of the environment variable for the test script --- I think this is actually the right thing to do here because nix will automatically set the environment variable for you, which I think is better than having an argument to the test script that you have to know to set on nix systems. The other is a stylistic concern, which I'm happy to defer to your judgement entirely on. Yes, I had gotten this to work under nix a week or so ago with all of the test cases passing, and just got the time to clean this up on Friday. Only then did I see @RossSmyth's #1285, which seems to have had activity from between me getting things working locally and cleaning things up and making my PR. I commented on that PR when I noticed, but haven't heard back yet. |
36fe055
to
f19d17a
Compare
I tried this out and got this:
Building |
Yes! This is expected (though maybe not ideal) and the
You can pull these from the
I'm not sure if there's a better solution in general, there's some discussions about this problem in nix: I'm not super familiar with the whole |
I added the flags in Also, |
You might want to also add the -I flags for |
Ok, finally figured it out! Turns out I was almost there, I just needed to change this arg
to say |
Fantastic! Yeah, I'm not sure, but it might make sense to be able to add some flags for the includes to c2rust directly, and then we can make a wrapper like the nix clang wrapper that automatically includes some of the system libraries. I figured for now it makes sense to just explicitly include all of the stuff because thats sort of what it seems like you're supposed to do, but it's definitely not the most convenient and the default compile_commands.json generated by the c2rust translator tests leaves some things implicit, I think. |
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.
A couple little things. I thought I sent this earlier but I guess I forgot to click submit.
This commit adds a number of improvements to the nix build for c2rust the main highlights are: - A c2rust package is provided, allowing nix users to install and run c2rust itself, instead of just getting a development environment from nix. - Update LLVM / clang to version 18. - Test cases pass in the c2rust dev environment now. Some fixes include... - Removing oxalica and just using fenix. Mixing rust overlays in nix was causing issues. - Use the nix tinycbor package to build c2rust. The c2rust cmake files add an external dependency on tinycbor, which causes sandboxed nix builds to fail because they cannot fetch the package. This involves patching the CMakeLists.txt file, and using pkg-config to find the system tinycbor library. Note: in the dev shell cmake will still fetch the external tinycbor. - Use the C2RUST_TINYCBOR_PATH environment variable, if it exists, to find tinycbor in c2rust-ast-exporter's build.rs - Use bindgenHook to make sure Rust's bindgen can find header files. - Update the test_translator.py script to include nix flags in compiler_commands.json Messing with bindgenHook, disable checks so package builds. - test_translator.py will use nix dependencies when the environment variable C2RUST_USE_NIX=1 + Passing the --use-nix flag to test_translator.py will also explicitly force the script to use nix dependencies.
Is this ready to be merged now? I'm on Nix, and have a few PRs going that are really awkward to work with without these fixes! :D |
@Chobbes When running some of the tests, I get errors saying it can't find EDIT: I think it's in this function somewhere, which generates compile commands for the tests. Since your changes added Nix-specific options to the compile commands elsewhere, I figure they need to be added here too? |
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.
Thanks! LGTM now!
Yes, I think so.
Unless something is broken. If it's all good, I can merge. |
It seems like it might be a Nix-specific issue relating to the tests. The PR as it stands now is an improvement for sure, and I'd be happy to see it merged now, but hopefully Chobbes will look into this problem afterwards. |
@Rua I can't look today, but I can look on Monday. All of the tests were passing for me with cargo in the dev shell, though. How are you running the tests? |
|
@Rua thanks! I guess I had been running the |
@Rua ah, yeah, okay, I didn't know about these cargo test cases. It looks like these rely on the |
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.
@Rua ah, yeah, okay, I didn't know about these cargo test cases. It looks like these rely on the
create_temp_compile_commands
function inc2rust-transpile/src/lib.rs
file to create thecompile_commands.json
file. I think this needs to be modified in a similar way to the python test script, or maybe there's a better way to wrap things so we don't need to do that. I'm not sure what the best approach is.
Feel free to remove that create_temp_compile_commands
in snapshots.rs
and replace it with a deliberately created compile_commands.json
. We've been meaning to do that for a few things (easier to compile as a crate so we can use dependencies and so we can suppress C warnings), so if that makes it easier for you, too, win-win.
This commit adds a number of improvements to the nix build for c2rust the main highlights are:
Some fixes include...