-
Notifications
You must be signed in to change notification settings - Fork 38
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
automatically patch binaries for nixos #48
Conversation
I saw in this comment that there are some issues with the patching I implemented above modifying checksums. @roynalnaruto will further changes be needed to account for that? |
cc @shazow this might be interesting for you. |
@d-xo Exciting, thank you for the ping! |
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.
Thank you for this PR, overall looks good to me :)
Just a small suggestion WRT refactoring. Also you will need to fix formatting/clippy warnings.
It would be great to have some additions to the CI specific to NixOS as well (I am not sure what to add there coz I don't use NixOS).
Further improvements can be made in following PRs, that's fine!
Again, thanks for the contribution :)
I made the requested change and fixed the formatting. I looked into CI, but github does not offer a nixos hosted runner, so I'm not sure how we can test this in CI without spinning up a custom nixos build machine :/ After testing a bit more locally I noticed that solc binaries <0.8 are already fully static and do not need to be patched, so I also made a small modification to reflect this. |
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! Thank you @d-xo
Closes #36 |
@@ -43,6 +44,9 @@ pub static SVM_HOME: Lazy<PathBuf> = Lazy::new(|| { | |||
/// The timeout to use for requests to the source | |||
const REQUEST_TIMEOUT: Duration = Duration::from_secs(60); | |||
|
|||
/// Version beyond which solc binaries are not fully static, hence need to be patched for NixOS. | |||
static NIXOS_PATCH_REQ: Lazy<VersionReq> = Lazy::new(|| VersionReq::parse(">=0.8.0").unwrap()); |
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.
This fails with solc 0.7.6, which seems to also be dynamically linked. Maybe we should patch every file in NixOS? statically linked ones should be noop
Implements #36.
This works by using
patchelf
to modify the dynamic linker in the installed solc binaries to use the nixos provided one (instead of the standard/lib64/ld-linux-x86-64.so.2
which is not present on nixos).Currently I've only tested locally with a few versions, and afaict it's working OK. I'm not sure what the automated testing strategy here is, but I would be more than happy to add some nixos specific tests / CI steps if you want.
Some more work will be needed to point the binaries to a nix provided version of z3/cvc (needed for the SMTChecker to work), but I'm going to leave that for another PR.