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

Nix flake #283

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

nicholaschiasson
Copy link

@nicholaschiasson nicholaschiasson commented Nov 15, 2024

Description

  • Adding initial nix flake capable of building the code
  • Checking Cargo.lock into version control

Testing

N/A since there is no code change.

I simply ran nix run locally to test that it is working.

Checklist

Things need to be done before this Pull Request can be merged.

  • Review if we want to expand upon dev shell, adding extra tooling for local devel/opment for those who wish to use nix
  • Review pinned SHA for fastnbt dependency and if this is necessary (not preferred since there is a manual step introduced in updating the flake in case we ever want to upgrade this dependency).
  • Review version in buildRustPackage, which is currently simply pulled from ./pumpkin/Cargo.toml. This is probably fine, except that it could be misleading about the outputs being built, masquerading the output as a proper release, when the build is coming from a feature branch. Maybe better would be to derive version somehow from git ref at HEAD or something.

@nicholaschiasson
Copy link
Author

For fastnbt dep, I understand now the reason it is necessary to pin the SHA in this flake is because the dependency is specified in a few Cargo.tomls using a git url. Is this because we need features not yet available in an official release (2.5.0), that are only in fastnbt master branch? And as a side question, is this a blocker before pumpkin itself can also have a stable release?

With this change, I have checked the Cargo.lock into version control, since that is the recommendation for rust projects that output binaries. The flake itself depends on the lock file as well, so it kind of makes the Cargo.lock file necessary. If that is problematic, then this change may be inappropriate for this repo. However, I think it will also help here. And for new contributors as well, since we guarantee people are getting the same versions of dependencies after a fresh repo clone.

If we ever wish to update the fastnbt dependency, the flake file will need to be updated too to reflect the new hash (tedious), or if we can pin against a new fastnbt release from the crates registry, we can remove the explicit pinning from the flake. That would be best because then there would be no hard coded versions in there and the file would rarely/never require further modification.

@Snowiiii
Copy link
Owner

We are currently moving to an own NBT Crate. Should be done soon. Only some issues with arrays when deserializing.
#100

* Adding initial nix flake capable of building the code
* Checking Cargo.lock into version control
* Room to improve nix flake dev shell
@Snowiiii
Copy link
Owner

I decided to partially move to our own NBT crate due to a Problem with Deserialization when having Arrays. Until the Problem is not solved we still need to use fastnbt for reading anvil chunks

@IogaMaster
Copy link

I +1 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.

3 participants