Nix refactor #1285
Closed
Nix refactor #1285
+317
−209
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kkysen
reviewed
Jul 28, 2025
There was a problem hiding this comment.
Hey @RossSmyth, thanks for this PR, but we don't really have the capacity to review nix improvements. If you can finish this up so it's not a draft and it looks generally okay, we can try to merge it, but we won't be able to help or review it with any depth.
|
Sounds good. I've been slammed at work so I've not been able to get back to this quite yet. |
cydparser
suggested changes
Aug 18, 2025
| python3 | ||
| }: | ||
| let | ||
| # Something changed in nixpkgs since this snapshot, |
There was a problem hiding this comment.
We can use a newer snapshot if the derivation is changed to only build c2rust:
cargoBuildFlags = [
"-p"
"c2rust"
];There was a problem hiding this comment.
What is the MSRV of c2rust? The whole point is tracking what the repo does.
There was a problem hiding this comment.
At the moment, 1.65 (the pinned nightly). Not sure if it works on any older versions.
There was a problem hiding this comment.
Yes, I realize that. cydparser said that if you only build c2rust you can "use a newer snapshot". I'm unsure how, so I am asking what they mean.
DieracDelta
reviewed
Aug 18, 2025
And route all instantiations through the overlay.
|
The tests still fail though |
|
@RossSmyth oh shoot, I didn't see this. I made a PR to fix up the nix build as well, didn't mean to step on any toes! I did manage to get all of the tests passing under nix as well, if that's helpful for you. |
|
@RossSmyth / @kkysen I'm wondering if you think it makes sense to merge #1341 first, as it's ready to go, and then the additional refactoring in this PR can be merged on top of it later? The shell.nix / README changes / flake checks / CI changes / general refactoring here are valuable, but maybe it makes sense to stage it so we can have a working nix package / shell now? I'm happy to help with merging these changes with mine after the fact! Thanks! |
kkysen
reviewed
Aug 29, 2025
There was a problem hiding this comment.
@RossSmyth / @kkysen I'm wondering if you think it makes sense to merge #1341 first, as it's ready to go, and then the additional refactoring in this PR can be merged on top of it later?
The shell.nix / README changes / flake checks / CI changes / general refactoring here are valuable, but maybe it makes sense to stage it so we can have a working nix package / shell now? I'm happy to help with merging these changes with mine after the fact!
I don't really have a strong opinion either way. I think whichever one is ready first can merge first, unless it's much more difficult to rebase one of them or something like that.
|
I'd say merge the other PR, but if I ever come back around to this (I am very busy with work and life and unfortunately this is low priority for me) I'll clean it up as it isn't quite idiomatic nix. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello,
This nix devshell was a bit jank. I cleaned it up, and also added a nix derivation.
This is fine because the version in nixpkgs is the same version that was being fetched via git anyways.
This provides things that c2rust needs in its environment to run, such as setting up PATH and other things.
This is basically the same as before. It is hooked up to the flake too.
This allows to check that it builds when updating and other things.
This is very useful, because it allows for people to directly import c2rust as an overlay into their NixOS config, home-manager config, or devshells.
I am unsure what sort of scope the project would like for Nix integration. This is basically the minimum of being able to build it on Nix easily. There are other things that could be done:
There is a TODO for a test failures:
c2rust/c2rust-analyze/tests/analyze.rs
Line 47 in 2120093
And various filecheck failures. It also looks like there is some sort of race happening on stderr output?
filecheck.log