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

Package Hermes #5

Merged

Conversation

JonathanLorimer
Copy link
Collaborator

@JonathanLorimer JonathanLorimer commented Jul 21, 2021

Was able to get hermes to build and run due to a crate2nix branch that fixes some of the underlying issues I was running into.

Solves #4

@JonathanLorimer
Copy link
Collaborator Author

Github actions are failing because of a known bug with flakes and import from derivation NixOS/nix#4265 (comment)

@JonathanLorimer JonathanLorimer changed the title Hermes is building and running Package Hermes Jul 21, 2021
@JonathanLorimer
Copy link
Collaborator Author

Okay, I think the best approach is to create a flake per project we want, and import them all into the top level flake!

@JonathanLorimer
Copy link
Collaborator Author

Okay, I switched to sub-flakes (which I think is nice, because then people can just point to that url if they want a subflake only). It still didn't resolve the import from derivation issue. This only breaks the commands nix flake show and nix flake check, which kinda sucks but I am gonna punt on this till later.

@JonathanLorimer
Copy link
Collaborator Author

SORTED OUT THE ISSUE WITH IFD. Its a bit of a hack, but should get things rolling for now, and as long as the flake is cached it shouldn't be a problem. Hopefully this gets fixed upstream.

flake.lock Outdated
"type": "github"
}
},
"nixpkgs_4": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know why Nix Flake is generating many versions of the same package in the lock file?

hermes/flake.nix Outdated
hermes =
let
name = "ibc-rs";
# The use of `evalPkgs` here is a major hack. This is to get around the IFD limitation in nix flakes
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am getting a warning:

trace: warning: crate2nix: Passing `buildRustCrate` as argument to Cargo.nix is deprecated. If you don't customize `buildRustCrate`, replace `callPackage ./Cargo.nix {}` by `import ./Cargo.nix { inherit pkgs; }`, and if you need to customize `buildRustCrate`, use `buildRustCrateForPkgs` instead.

Is it related to this workaround?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah, that's because I need to use a fork of crate2nix. Right now crate2nix does not use fetchGit under the hood, so it only looks for the master branch. ibc-rs uses main I believe.

crate2nix = {
      url = "github:yusdacra/crate2nix/feat/builtinfetchgit";
      flake = false;
    };

Here is the relevant issue upstream nix-community/crate2nix#205

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. I thought the input for flakes is fixed, so inside a flake the Nix code shouldn't able to do fetchGit directly? Is the argument ibc-rs-src in the output closure just a URL, or is it a derivation containing the fetched source code already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, crate2nix isn't a flake. I know they keep track of git resource hashes using a json file.

Here is the relevant code: https://github.com/yusdacra/crate2nix/blob/master/tools.nix#L234-L241

Copy link
Collaborator

@soareschen soareschen left a comment

Choose a reason for hiding this comment

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

Works great on my machine!

hermes/flake.nix Outdated
};

# nix build
defaultPackage = packages.hermes;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better to not provide a default package? I'd imagine as we add new projects, the user might not want to build or run hermes by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I agree for the top level flake, gonna make that change. I think for the hermes/flake.nix the user probably just wants nix run to invoke the ibc-relayer-cli. I am interested to hear your thoughts here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I'm referring to the top level project. Got mixed up which file I'm looking at.

hermes/flake.nix Outdated

# nix run
apps = {
hermes = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add documentation on README.md on what commands can be run? It is pretty tricky for new users to figure out what Nix commands to run. From here I can see two commands available: nix build .#hermes and nix run .#hermes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a good idea!

@JonathanLorimer
Copy link
Collaborator Author

@soareschen no rush on this, but its ready for re-review. Also sorry this has all taken so long. I didn't realize that nix + rust was so complicated (perhaps just bias since I am sure newcomers to cabal2nix would have the same complaint). FWIW I think the go projects will be easier to package (it makes me kinda sad to say that lol).

@soareschen
Copy link
Collaborator

When I first tried running the updated branch from the project root I got the error:

$ nix run .#hermes
error: NAR hash mismatch in input 'path:./hermes?narHash=sha256-Ug%2fOjuuCulhd79RCFDJLSMSPjcyvY1+XY6vwEFjaEPQ=' (/nix/store/z85653a74ggx4w9bzg8ppj
5q9m5dggff-source), expected 'sha256-Ug/OjuuCulhd79RCFDJLSMSPjcyvY1+XY6vwEFjaEPQ=', got 'sha256-Lnqh0y13bU6/7A4Rv7WM0aQzioLmkdJ8k3nh+V3CH8c='     
(use '--show-trace' to show detailed location information)

I had to go into the ./hermes directory and run nix run there first, then the error is gone after going back to the project root.

Could this be caused by local changes in the local ./hermes directory, which causes the checksum to change? Are gitignored files also tracked in the flake checksum?

@JonathanLorimer
Copy link
Collaborator Author

@soareschen I am having trouble reproducing locally, even after running nix store delete .#hermes. However, I did notice this same error occurring in the CI I setup in my personal repo. It seems strange that the hashes would be different, and I am having trouble tracking the source of nondeterminism. It could be from the use of fetchgit in crate2nix.

@JonathanLorimer
Copy link
Collaborator Author

Okay, I was able to reproduce by running these commands:

$> nix nar dump-path ./hermes > hermes.nar
$> nix hash-file --type sha256 ./hermes.nar
warning: 'hash-file' is a deprecated alias for 'hash file'
sha256-Lnqh0y13bU6/7A4Rv7WM0aQzioLmkdJ8k3nh+V3CH8c=
$> cd hermes
$> nix build
$> cd ..
$> nix nar dump-path ./hermes > hermes.nar
$> nix hash-file --type sha256 ./hermes.nar
warning: 'hash-file' is a deprecated alias for 'hash file'
sha256-Ug/OjuuCulhd79RCFDJLSMSPjcyvY1+XY6vwEFjaEPQ=

It appears we need to be careful to run nix flake lock --update-input <package-name> when there is a result directory in the dir below. I am going to looking into using this https://github.com/hercules-ci/gitignore.nix to ignore the results dirs when computing the nar hash.

@JonathanLorimer
Copy link
Collaborator Author

Confirmation that this issue appears to be resolved in CI https://github.com/JonathanLorimer/cosmos.nix/runs/3265578628?check_suite_focus=true

@JonathanLorimer
Copy link
Collaborator Author

Okay, I also realized I don't need the sub directories to be flakes, they can just be regular derivations.

@JonathanLorimer JonathanLorimer mentioned this pull request Aug 9, 2021
@JonathanLorimer
Copy link
Collaborator Author

@soareschen I think this is ready for re-review whenever you have a moment. Thanks for taking so much time to look through this!

Copy link
Collaborator

@soareschen soareschen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks and sorry for the delay in reviewing!

@soareschen soareschen merged commit 38555f0 into informalsystems:master Aug 16, 2021
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