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

Update the version of Rust used to 1.64 for all pkgs after 2022-09-23 #23

Merged
merged 1 commit into from
Sep 24, 2022

Conversation

mitchmindtree
Copy link
Contributor

This should account for FuelLabs/fuel-core@bcb86da while still allowing to use 1.63 (and in turn, already cached builds) for prior versions.

This should account for FuelLabs/fuel-core@bcb86da while still allowing to use 1.63 (and in turn, already cached builds) for prior versions.
@mitchmindtree mitchmindtree requested a review from a team September 23, 2022 09:46
@mitchmindtree mitchmindtree self-assigned this Sep 23, 2022
@mitchmindtree mitchmindtree enabled auto-merge (squash) September 23, 2022 10:32
@Voxelot
Copy link
Member

Voxelot commented Sep 23, 2022

Hmm doesn't seem to be working for me 🤔

voxelot@ryzerator:~/code/fuel/fuel-core$ nix develop github:fuellabs/fuel.nix/mitchmindtree/update-rust-version#fuel-core-dev
voxelot@ryzerator:~/code/fuel/fuel-core$ rustc --version
rustc 1.63.0 (4b91a6ea7 2022-08-08)
voxelot@ryzerator:~/code/fuel/fuel-core$ exit
exit
voxelot@ryzerator:~/code/fuel/fuel-core$ rustc --version
rustc 1.64.0 (a55dd71d5 2022-09-19)

@mitchmindtree
Copy link
Contributor Author

mitchmindtree commented Sep 23, 2022

Hmm doesn't seem to be working for me

Ahhhh, that's because the latest published fuel-core instance precedes the 2022-09-23 date add in this PR, and its fuel-core's build inputs that are provided to the fuel-core-dev shell.

I think there are two things that we can do:

  1. Change fuel-core-dev to use fuel-core-nightly's build inputs rather than fuel-core's to make sure devs get the current dev environment, not the environment for the last published release. Addressed in Use nightly package build inputs to ensure devShells have up-to-date tools #26.
  2. Address GitHub Actions user needs permission to push to master branch for automated refresh-manifests CI #22 to make sure we have the latest nightlies. I might just open a PR to update these manually until GitHub Actions user needs permission to push to master branch for automated refresh-manifests CI #22 gets solved. PR Manually run nix run .#refresh-manifests, adds latest nightlies #27.

mitchmindtree added a commit that referenced this pull request Sep 23, 2022
…tools

Previously, the devShells provided the build inputs and environment from
the latest *semver* release of their respective packages. This meant
that the devShells could fall behind the current development
requirements.

This changes the devShells to instead provide the build environment of
their respective *nightly* packages, ensuring that the devShells are at
least up-to-date within 24 hrs.

Issue previously spotted [here](#23 (comment)).
mitchmindtree added a commit that referenced this pull request Sep 23, 2022
…tools

Previously, the devShells provided the build inputs and environment from
the latest *semver* release of their respective packages. This meant
that the devShells could fall behind the current development
requirements.

This changes the devShells to instead provide the build environment of
their respective *nightly* packages, ensuring that the devShells are at
least up-to-date within 24 hrs.

Issue previously spotted [here](#23 (comment)).
@Voxelot
Copy link
Member

Voxelot commented Sep 23, 2022

Is there a way we could specify a specific revision instead of a date? As I imagine this will make the nix shell somewhat unstable when rust updates occur since devs would have to wait for a nightly build to convince the shell to use the correct rust version.

@Voxelot
Copy link
Member

Voxelot commented Sep 24, 2022

Also if we plan on being able to use these dev-shells in CI due to the improved build caching, we'll need to support a different way to configure the version of rust to use. Since this current approach would lead to a chicken and egg problem between this flake and fuel-core when trying to upgrade the Rust version in the future.

@mitchmindtree
Copy link
Contributor Author

Is there a way we could specify a specific revision instead of a date? As I imagine this will make the nix shell somewhat unstable when rust updates occur since devs would have to wait for a nightly build to convince the shell to use the correct rust version.

I went with using the fuel package's manifest date as currently the repo only refreshes these manifests once a night, and in turn only learns about new fuel package versions and nightlies once a night. We could make this more frequent (or potentially hook into events from our other repos somehow? Never worked out how to do this for fuelup), though this would still require a new nightly or semver release to exist before fuel-core-dev would get the new Rust version.

I'm not sure if there's an easy way to override the Rust version from the command line 🤔


It is possible by wrapping fuel-dev or fuel-core-dev with your own Nix shell. E.g. I use a personal devShell that wraps fuel-dev providing some extra tools, sets my git configuration, etc like so:

{
  fuel-dev,
  pkgs,
}:
pkgs.mkShell {
  name = "fuel-dev-env";
  # Required inputs come from the `fuel-dev` devShell.
  inputsFrom = [fuel-dev];
  buildInputs = with pkgs; [
    cargo-bloat
    cargo-deps
    cargo-generate # For sway integration test generation.
    graphviz
    libusb # in case of hardware wallet stuff.
    mdbook
    pkgconfig
    rust-analyzer
  ];
  shellHook = ''
    # Use fuel email for fuel-related dev.
    export GIT_AUTHOR_EMAIL='mitchell.nordine@fuel.sh'
  '';
  inherit (fuel-dev) LIBCLANG_PATH ROCKSDB_LIB_DIR PROTOC NIX_CFLAGS_COMPILE;
}

One could add a newer Rust version to the buildInputs which should replace the instance provided by fuel-dev.


Also if we plan on being able to use these dev-shells in CI due to the improved build caching, we'll need to support a different way to configure the version of rust to use. Since this current approach would lead to a chicken and egg problem between this flake and fuel-core when trying to upgrade the Rust version in the future.

True, in general we probably want to avoid having this kind of cyclic dependency between repositories either way.

I think the proper approach might be for fuel-core to have it's own flake and devShell, in which it can specify it's own Rust version and whatever else as necessary on a per-commit basis.

I've only really included the fuel-core-dev, sway-dev and fuel-dev devShells in fuel.nix (rather than upstreaming them to the fuel-core and sway repos) in order to make them available to those interested in Nix without having to "pollute" the upstream repos with Nix files that might be unfamiliar/scary to some Fuel devs :)

That said, if the fuel-core team is keen, having the flake.nix (and in particular, the devShells) local to each repo and a part of those repos' git commit process would be a lot more idiomatic and give the fuel-core team to change/add/remove stuff as needed without the nightly delay.

Ideally, fuel.nix would just be a platform for using all of the Fuel+Sway packages (similar to what rust-overlay provides for Rust), and provide tooling for building and running Fuel+Sway applications. It probably shouldn't be concerned with providing dev shells for building the upstream tools themselves.

@Voxelot
Copy link
Member

Voxelot commented Sep 24, 2022

I think the proper approach might be for fuel-core to have it's own flake and devShell, in which it can specify it's own Rust version and whatever else as necessary on a per-commit basis.

This makes a lot of sense, and would work better with nix plugins for IDE's and the like. E.g. https://github.com/nix-community/nix-direnv#nix-direnv

@mitchmindtree
Copy link
Contributor Author

Let's land this in the meantime anyways so that the upcoming nightlies build :)

@mitchmindtree mitchmindtree merged commit 2b5c45a into master Sep 24, 2022
@mitchmindtree mitchmindtree deleted the mitchmindtree/update-rust-version branch September 24, 2022 02:15
mitchmindtree added a commit that referenced this pull request Sep 24, 2022
…tools (#26)

Previously, the devShells provided the build inputs and environment from
the latest *semver* release of their respective packages. This meant
that the devShells could fall behind the current development
requirements.

This changes the devShells to instead provide the build environment of
their respective *nightly* packages, ensuring that the devShells are at
least up-to-date within 24 hrs.

Issue previously spotted
[here](#23 (comment)).
mitchmindtree added a commit that referenced this pull request Sep 24, 2022
It looks like my previous rust-overlay update in #23 was a little early
and didn't include 1.64 yet. #23 didn't show any issues yet as none of
the packages at the time of that commit were released after 2022-09-23,
and so we didn't hit the error until the latest nightlies merged into
`master` with #27 which also didn't show any issues as it was was opened
before #23 was merged :')

This should fix it! It's probably worth enabling the same branch
protection we have on the Sway repo where merging a PR requires it's up
to date with `master` to avoid cases like this.
mitchmindtree added a commit that referenced this pull request Sep 24, 2022
It looks like my previous rust-overlay update in #23 was a little early
and didn't include 1.64 yet. #23 didn't show any issues yet as none of
the packages at the time of that commit were released after 2022-09-23,
and so we didn't hit the error until the latest nightlies merged into
`master` with #27 which also didn't show any issues as it was was opened
before #23 was merged :')

This should fix it! It's probably worth enabling the same branch
protection we have on the Sway repo where merging a PR requires it's up
to date with `master` to avoid cases like this.
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.

2 participants