-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add dev helper and docs #574
Conversation
ec75364
to
f612a68
Compare
dev/create-dev-env.sh
Outdated
|
||
if [[ ! -e src ]]; then | ||
echo "Cloning fort-nix/nix-bitcoin" | ||
git clone https://github.com/fort-nix/nix-bitcoin src |
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 cloning, don't you want to cd
into it?
It seems to me that all this cloning (or not) etc. might just confuse people. Generally I would expect people who run it to already have gtt clone
d the repo manually. And then having that as pre-condition can be used to double check if the CWD is correct 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.
After cloning, don't you want to cd into it?
No, the setup script simply clones the nix-bitcoin repo to dir src
, without later changing its contents.
Generally I would expect people who run it to already have
git cloned
the repo manually
Yes, but if the script moved/renamed the user's existing files, it would be even more confusing.
Users can simply replace the cloned repo with their local clone.
The dev env layout is very opinionated; it's what I prefer for creating auxiliary scripts/notes/resources for a public repo.
But I'm very interested in suggestions on how to improve it.
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 dev env layout is very opinionated; it's what I prefer for creating auxiliary scripts/notes/resources for a public repo.
But I'm very interested in suggestions on how to improve it.
Just thinking aloud, I'm not even all that familiar with nix-bitcoin, or generally developing strictly Nix-focused project (as opposed to a Rust project, that just uses Nix for building and tooling).
Generally if the project includes it's own tests and dev-env integration, I would expect git clone <project-repo> && cd <project> && nix develop (or direnv)
to just set up everything for me (add binaries to the $PATH, create some repo-scoped configs and so on).
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.
Just thinking aloud
Highly appreciated, thanks!
create some repo-scoped configs
Yeah, like our scenarios.nix
file. How exactly would you implement this? Where would these files be created? (Note that they must be placed in a separate repo).
Adding an extra dir in the repo (another approach that I've used in the past) complicates repo-wide search (and replace), because the extra content is included.
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.
Note that you can develop nix-bitcoin just fine by simply running test/run-tests.sh
and editing tests.nix
.
I'll mention this in the docs.
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.
Yeah, like our scenarios.nix file. How exactly would you implement this? Where would these files be created? (Note that they must be placed in a separate repo).
Oh, I don't really understand how these works, but generally for files like these it's "if they don't exist, copy/ln from a template of some kind and have them in .gitignore
". Like here
The "must be placed in a separate repo" seems quite confusing to me, but I wouldn't spend too much time explaining it to me. :D
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.
must be placed in a separate repo
Sorry, that was misleading. What I meant is that files like scenarios.nix
should be put under version control, but, obviously, in an extra repo. This extra repo could simply be located inside the nix-bitcoin repo, but this has some downsides, as mentioned above.
652a277
to
3d74fb5
Compare
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.
Lots of cool stuff. Thanks! Left a few comments.
dev/dev.sh
Outdated
#――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― | ||
# Access nix-bitcoin flake packages | ||
|
||
nb() { | ||
nix build --no-link --print-out-paths --print-build-logs "$@" | ||
} | ||
|
||
# A package defined by nix-bitcoin | ||
nb .#joinmarket |
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 we run (parts of) the new shell scripts in ci?
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.
In the past, I've failed to directly run extra-container on the Cirrus CI node. I'll investigate this further and maybe later add tests for the dev scripts.
8faa337
to
04ebece
Compare
This removes a source of implicit state and guarantees that regular calls to `run-tests.sh` always run the builtin tests.
Previously, an incomplete clightning unit was always created because attr `clightning` was always defined in option attrset `systemd.services`.
Move line next to `services.lnd` config for clarity.
Rebased to master. |
1dbca95
to
b3665fc
Compare
LGTM now. Feel free to squash. |
f9349d8
to
b4d7e1a
Compare
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.
ACK b4d7e1a
Fixes #557.