Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Update the nix build configuration. #13706

Merged
merged 5 commits into from
May 20, 2023
Merged

Update the nix build configuration. #13706

merged 5 commits into from
May 20, 2023

Conversation

farcaller
Copy link
Contributor

@farcaller farcaller commented Mar 24, 2023

Remove the old shell.nix with some legacy versions pinned and replace it with a flake-based shell. It installs rust via rustup instead of fenix to be more generally compatible with the guidelines.

This also adds the rust-toolchain.toml spec with all the components required for wasm, and everything else to make rust-analyzer & clippy happy.

I verified that cargo build --release works for the node within the direnv shell created from the flake.

✄ -----------------------------------------------------------------------------

Thank you for your Pull Request! 🙏

Before you submit, please check that:

  • Description: You added a brief description of the PR, e.g.:
    • What does it do?
    • What important points should reviewers know?
    • Is there something left for follow-up PRs?
  • Labels: You labeled the PR appropriately if you have permissions to do so:
    • A* for PR status (one required)
    • B* for changelog (one required)
    • C* for release notes (exactly one required)
    • D* for various implications/requirements
    • Github project assignment
  • Related Issues: You mentioned a related issue if this PR is related to it, e.g. Fixes #228 or Related #1337.
  • 2 Reviewers: You asked at least two reviewers to review. If you aren't sure, start with GH suggestions.
  • Style Guide: Your PR adheres to the style guide
    • In particular, mind the maximal line length of 100 (120 in exceptional circumstances).
    • There is no commented code checked in unless necessary.
    • Any panickers in the runtime have a proof or were removed.
  • Runtime Version: You bumped the runtime version if there are breaking changes in the runtime.
  • Docs: You updated any rustdocs which may need to change.
  • Polkadot Companion: Has the PR altered the external API or interfaces used by Polkadot?
    • If so, do you have the corresponding Polkadot PR ready?
    • Optionally: Do you have a corresponding Cumulus PR?

Refer to the contributing guide for details.

After you've read this notice feel free to remove it.
Thank you!

✄ -----------------------------------------------------------------------------

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Mar 24, 2023

User @farcaller, please sign the CLA here.

bin/node-template/flake.nix Outdated Show resolved Hide resolved
pkgs = import nixpkgs { inherit system; };
in
{
devShells.default = pkgs.mkShell {

Choose a reason for hiding this comment

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

devenv can be used here. it also generates Codespace on demand and more sembles home-manager/nixos config. but this is good too.

Copy link
Contributor Author

@farcaller farcaller Mar 27, 2023

Choose a reason for hiding this comment

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

Fixed, thanks! wrong comment

I'll look into it in the next PR if that's alright. I want to add the buildable into the flake as well and it's a bit tricky to do right with a sandbox right now.

@andresilva
Copy link
Contributor

The node-template README mentions using lorri, it should be updated to point to nix-direnv which this PR uses.

@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit C1-low PR touches the given topic and has a low impact on builders. labels Mar 28, 2023
@bkchr
Copy link
Member

bkchr commented Mar 28, 2023

The node-template README mentions using lorri, it should be updated to point to nix-direnv which this PR uses.

@farcaller please do this and then we can merge this!

Remove the old shell.nix with some legacy versions pinned and replace it with a flake-based shell. It installs rust via rustup instead of fenix to be more generally compatible with the guidelines.

This also adds the rust-toolchain.toml spec with all the components required for wasm, and everything else to make rust-analyzer & clippy happy.
@farcaller
Copy link
Contributor Author

@bkchr done. I also added the top-level configuration for substrate as a whole for those who will hack on the project itself (like me) and want the rust-analyzer to work out of the box in nix.

@bkchr
Copy link
Member

bkchr commented Mar 28, 2023

@bkchr done. I also added the top-level configuration for substrate as a whole for those who will hack on the project itself (like me) and want the rust-analyzer to work out of the box in nix.

Do you know if I block this in direnv, it will annoy me every time I open the folder? (I have my own nix files for Substrate etc)

@farcaller
Copy link
Contributor Author

I'm afraid the nagging will be there, yeah. I can make an optional env variable to opt out of that, if it works? My reasoning to have it is that direnv is pretty much the only way for vscode to be friendly with rust-analyzer in complex setups, but I'm not too attache to that commit and can carry it around with me.

@bkchr
Copy link
Member

bkchr commented Mar 29, 2023

Yeah just tested it, could you maybe for now disable it? Or keep the nix file and rename the .envrc to .envrc_template

@farcaller
Copy link
Contributor Author

Will do. I’ll add .envrc to the ignores too, then.

@farcaller
Copy link
Contributor Author

Updated per comments.

rust-toolchain.toml Outdated Show resolved Hide resolved
@dzmitry-lahoda
Copy link

nixification is essential for cross chain contracts and bridges development

rust-toolchain.toml Outdated Show resolved Hide resolved
bin/node-template/flake.nix Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented May 10, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added A3-stale and removed A3-stale labels May 10, 2023
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2823048

@bkchr
Copy link
Member

bkchr commented May 19, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkchr
Copy link
Member

bkchr commented May 19, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for 07465c6

@bkchr bkchr merged commit 2000147 into paritytech:master May 20, 2023
@farcaller farcaller deleted the nix branch May 20, 2023 15:38
gpestana pushed a commit that referenced this pull request May 27, 2023
* Update the nix build configuration.

Remove the old shell.nix with some legacy versions pinned and replace it with a flake-based shell. It installs rust via rustup instead of fenix to be more generally compatible with the guidelines.

This also adds the rust-toolchain.toml spec with all the components required for wasm, and everything else to make rust-analyzer & clippy happy.

* Also add the top level flake for hacking on the substrate as whole

* Remove the envrc and ignore it instead.

* Remove the top-level configuration

---------

Co-authored-by: parity-processbot <>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Update the nix build configuration.

Remove the old shell.nix with some legacy versions pinned and replace it with a flake-based shell. It installs rust via rustup instead of fenix to be more generally compatible with the guidelines.

This also adds the rust-toolchain.toml spec with all the components required for wasm, and everything else to make rust-analyzer & clippy happy.

* Also add the top level flake for hacking on the substrate as whole

* Remove the envrc and ignore it instead.

* Remove the top-level configuration

---------

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants