-
Notifications
You must be signed in to change notification settings - Fork 53
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
Nixfying #536
base: main
Are you sure you want to change the base?
Nixfying #536
Conversation
add commands and dev shell
it was not applied before verifying the lock
note: this commands are not fully tested. can be adjusted when needed.
Thanks @countoren for nixifying 👍 I am testing your branch right now, but bumping into some errors. nix-shell
error:
… while calling a functor (an attribute set with a '__functor' attribute)
at /Users/afsalthaj/github/nix/golem/default.nix:9:1:
8| }:
9| pkgs.rustPlatform.buildRustPackage {
| ^
10| pname = "golem-cli";
error: assertion '((cargoVendorDir == null) -> (! ((cargoSha256 == "") && (cargoHash == ""))))' failed
at /nix/store/01sqyyi3b57wh47aax0mzyidyzg08qsk-nixpkgs-21.05pre273666.d1257435332/nixpkgs/pkgs/build-support/rust/default.nix:58:1:
57|
58| assert cargoVendorDir == null -> !(cargoSha256 == "" && cargoHash == "");
| ^
59| assert buildType == "release" || buildType == "debug"; |
Any updates on this @countoren ? We would like to get this in, just that I was having trouble testing this branch. #536 (comment) |
Hi @afsalthaj, I missed the first comment,
instead of nix-shell |
I think i was able to replicate the issue I will try to fix it this weekend. |
Just add missing explicit import
to get the a result with golem cli
to jump into the dev shell |
@countoren Thank you for this. I will try nix soon and if works, we will get it merged. |
It hasn't worked for me. I have not been following nix for more than a year. That said, it's going to be harder for developers and maintain this, as it seems it may work in 1 machine and not in the other.
|
@countoren Nix-shell worked for me '/nix/store/ma6za5vicwdys9jr5qbfpfc3m88z0j9w-wasmtime-c2e97ff/crates/wasi-common/WASI/tools/witx/src/ast.rs' -> '/nix/store/bdsf6ihs4yqfq5zyrikf2hjf7mdvyyc1-witx-0.9.1/src/ast.rs'
'/nix/store/ma6za5vicwdys9jr5qbfpfc3m88z0j9w-wasmtime-c2e97ff/crates/wasi-common/WASI/tools/witx/src/validate.rs' -> '/nix/store/bdsf6ihs4yqfq5zyrikf2hjf7mdvyyc1-witx-0.9.1/src/validate.rs'
'/nix/store/ma6za5vicwdys9jr5qbfpfc3m88z0j9w-wasmtime-c2e97ff/crates/wasi-common/WASI/tools/witx/src/parser.rs' -> '/nix/store/bdsf6ihs4yqfq5zyrikf2hjf7mdvyyc1-witx-0.9.1/src/parser.rs'
'/nix/store/ma6za5vicwdys9jr5qbfpfc3m88z0j9w-wasmtime-c2e97ff/crates/wasi-common/WASI/tools/witx/src/phases.rs' -> '/nix/store/bdsf6ihs4yqfq5zyrikf2hjf7mdvyyc1-witx-0.9.1/src/phases.rs'
building '/nix/store/k0g60rs0b0vc68fakhbjnaz3a19cw3vl-cargo-vendor-dir.drv'...
[nix-shell:~/projects/nixed/golem]$
|
Not sure So in short, nix-shell finally worked for me, and not nix-develop |
@@ -0,0 +1,10529 @@ | |||
# This file is automatically @generated by Cargo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is generating this copy of our Cargo.lock file and what happens when we change our dependencies (which is quite frequent?)
, golem-examples ? pkgs.fetchFromGitHub { | ||
owner = "golemcloud"; | ||
repo = "golem-examples"; | ||
rev = "5165dcc7e3cbfa09f752caa96a869d284ec169aa"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this revision? What happens when we update golem-exampels
? (we do it quite frequently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this revision is just for "testing" this parameter will be overwritten by flake, this is used in nix-build/nix-shell only
flake suppose to be the default use (nix build) look at the comment at the flake inputs to see how to keep update.
}: | ||
pkgs.rustPlatform.buildRustPackage { | ||
pname = "golem-cli"; | ||
version = "0.0.98"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is outdated now - who and when would update it? We are not keeping our version numbers anywhere else in our repo but use git tags to do releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be set to 0.0.0, I guess it will be possible to set it in a shellScript command before build from a tag but that will count as kinda of "side effect" and then you will have issue for people that will want to run:
nix build github:golemcloud/golem
without cloning it.
cargoLock = { | ||
lockFileContents = builtins.readFile ./Cargo.lock.nix; | ||
allowBuiltinFetchGit = false; | ||
outputHashes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is special about these two dependencies? What if they need to be updated? Or how do we know we need them or need something else here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not 100% sure why nix could not include this dependencies directly from lock file, I had to include them explicitly to resolve some conflicts.
in order to update them it could be done manually if they are not updated that frequently otherwise it is possible to add another command to update-deps command in commands.nix (dev shell)
) | ||
.unwrap(); | ||
|
||
- println!("cargo::rerun-if-changed=build.rs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file tells me that this nix package is trying to build Golem with an older rust version (the ::
syntax is since 1.77). We don't want to pin our codebase to an old rust version, and are potentially using new rust features so even though we could change this particular syntax back but the build can break any time if it's not using the same rust version that we are using (which is, always the latest, for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to run an update (remove/update flake.lock) and update the dependencies we might wont need this line anymore.
@@ -0,0 +1,37 @@ | |||
{ | |||
inputs.golem-examples.url = "github:golemcloud/golem-examples"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is golem-examples
special for nix, and not our other dependencies? (For example golem-wasm-rpc
, golem-wasm-ast
, golem-wit
, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had issue building golem-examples when building golem-cli from nix
I think it was more "side effecty" setup.rs then the other projects, it was missing "Inflector" package. I was able to solve it referencing it on the nix build of golem-cli.
update-deps in the dev shell (commands.nix) will update it to latest version
@@ -0,0 +1,82 @@ | |||
[package] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a copy of our golem-cli
cargo file here? Who and when will keep it updated? What is the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the difference between this file and Cargo.toml is the change to of golem examples to be to one that is brought by nix (inputs.golem-examples, and additional missing reference),
I think I had planned to be included in the update-deps command on the commands.nix but i might missed something I could look at it this weekend.
name = "golem-cli" | ||
version = "0.0.0" | ||
dependencies = [ | ||
+ "Inflector", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this dependency added by a patch here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I had to include it to build correctly golem-examples but I might be wrong I will check it this weekend
it was seatching for devShell did you try |
@countoren @vigoo I have been packaging golem not being aware of this branch and I have proposed some changes to upstream The package expression can be found here NixOS/nixpkgs#344289 Having golem in nixpkgs would also not require you to maintain Nix inside of this repository |
No description provided.