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

fix nix environment #1294

Merged
merged 4 commits into from
Jan 3, 2023
Merged

fix nix environment #1294

merged 4 commits into from
Jan 3, 2023

Conversation

dbarrosop
Copy link
Contributor

@dbarrosop dbarrosop commented Dec 10, 2022

In addition to the issue described in #1292 it also sets correctly RUST_SRC_PATH and patches rustlings lsp to make use of it if available. Otherwise the path for sysroot_src is computed wrongly making rust-analyzer fail.

@dbarrosop dbarrosop changed the title fix nix build on Darwin fix nix environment Dec 10, 2022
Copy link

@luxus luxus left a comment

Choose a reason for hiding this comment

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

tested the pr and i was able to use nix develop

@2gn 2gn mentioned this pull request Dec 14, 2022
buurro referenced this pull request in buurro/rustlings Dec 19, 2022
@buurro
Copy link

buurro commented Dec 19, 2022

I was testing this and cargo run lsp wouldn't run without CoreServices in the devShell buildInputs.

I fixed it with this commit

@dbarrosop
Copy link
Contributor Author

@buurro that's because we are mixing installing software with nix and non-nix mechanisms. Ideally we'd installing everything with nix and then this wouldn't be a problem. However, as this is a learning tool mixing mechanisms might be ok, it depends a bit on the goal maintainers had when introducing nix here. Would be nice to hear from a maintainer to know what they think.

@shadows-withal
Copy link
Member

Not sure what exactly the problem is here, but if you want my rationale, I merged the original PR so that people with Nix installed could just run nix develop and have a shell they could work in. I'm not well versed in "the nix way", so I kinda had to rely on other people to make the changes, which is why I don't fully understand the issue at hand here.

If this PR fixes the flake for people that've had trouble working with this, though, then I'd be inclined to merge it.

@dbarrosop
Copy link
Contributor Author

Thanks for the clarification, so there are two main problems currently with the nix setup:

  1. It doesn't work on Darwin
  2. Anything that relies on the NIX_SRC_PATH is also broken (i.e. rust-analyzer)

This PR fixes both issues.

As this is simply about having an easy to deploy environment I think we can forget about the nix way and install everything that is required to build rust applications using cargo so I just pushed another commit to add the required dependencies on macos. I also added rustfmt and clippy as those complement rust-analyzer but we can remove those if you prefer.

@shadows-withal
Copy link
Member

Since I can test this on Darwin, and main is indeed broken, and this fixes it, that's good enough for me to merge. Thanks!

@shadows-withal shadows-withal merged commit 1426335 into rust-lang:main Jan 3, 2023
@shadows-withal
Copy link
Member

@all-contributors please add @dbarrosop for infra

@allcontributors
Copy link
Contributor

@diannasoreil

I've put up a pull request to add @dbarrosop! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants