-
Notifications
You must be signed in to change notification settings - Fork 21
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
Adds holo-dev-server into the nix develop shell if scaffold is holo-enabled #162
Conversation
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.
Please could the tests be extended to cover this new code?
src/scaffold/app/nix.rs
Outdated
]; | ||
|
||
shellHook = '' | ||
nix-env -f "https://hydra.holo.host/channel/custom/holo-nixpkgs/alpha/holo-nixpkgs/nixexprs.tar.xz" -iA holo-dev-server |
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 this is potentially surprising for updated versions of a binary to be installed on starting a shell. Is there another way of doing this so it's included as a package and locked with the flake?
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.
Should it be locked with the flake? This binary should represent production Holo Network, and that can change separately to the rest of the flake.
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.
That said, adding it in this way seems ugly to me. I just don't know a better way
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.
After discussing I understand why this is hard to change. With the source code being closed source it's not simple to build from source and hydra is giving us only the latest binary per channel so we don't have an obvious way to lock to a specific version. It would be good to solve this at some point so that people can look at the flake.lock and see what version they have, and when it gets updated. This also makes offline development difficult but for now the install line could be commented out temporarily and the binary manually installed before going offline or some other workaround.
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.
Pre-emptively filed an issue: #163
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 alters the user's profile permanently and not just for the duration of the nix shell. what is the chance to open-source the holo-dev-server?
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.
eek
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 will be a long discussion and I don't know what the outcome will be
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.
packaging closed-source binaries isn't great but it's feasible. it does require some form locking, which can be automatically maintained on a separate public repo and consumed here. given that the scaffolding performs a nix flake update
makes the consumption of it a non-issue.
The test looks good to me, and the bash logic to check if the program is available looks sound to me. Not sure why it's not passing though? |
@steveej Hopefully we can remove that |
@alastairong I think the changes in #165 can also be included in this PR, seeing they are very similar |
@c12i the other PR is a duplicate as I was testing something. Sorry for the confusion |
Deprecating in favour of #181 as Numtide is adding their commits there. |
For holo-enabled scaffolds we need to make holo-dev-server available to the developer for app testing.
This PR makes it available via the nix shell.