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.

Sign up for GitHub

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

Jump to bottom

Add holo-dev-server to the dev shell when --holo is passed #181

Merged
merged 3 commits into from
Mar 21, 2024
Merged

Add holo-dev-server to the dev shell when --holo is passed #181

merged 3 commits into from
Mar 21, 2024

Conversation

r-vdp
Copy link
Contributor

@r-vdp r-vdp commented Jan 15, 2024 •
edited
Loading

We add a --holo flag to the example subcommand.
When this flag is passed, we add holo-nixpkgs as a flake input and we add holo-dev-server to the dev shell's packages.

This PR builds on #165 that was opened by @alastairong

@r-vdp r-vdp marked this pull request as draft January 15, 2024 16:16
@r-vdp r-vdp force-pushed the add_holo_dev_server branch from 52b1d70 to 8d18755 Compare January 15, 2024 16:28
@r-vdp r-vdp marked this pull request as ready for review January 15, 2024 16:57
@c12i c12i self-requested a review January 15, 2024 18:06
@r-vdp r-vdp force-pushed the add_holo_dev_server branch from 8d18755 to 87e3f64 Compare January 15, 2024 20:34
@r-vdp
Copy link
Contributor Author

r-vdp commented Jan 15, 2024

@c12i I pushed a commit to hopefully fix the tests. Weird though that github does not show any log output for the failed runs.

c12i
c12i requested changes Jan 16, 2024
View reviewed changes
run_test.sh Outdated Show resolved Hide resolved
run_test.sh Outdated Show resolved Hide resolved
run_test.sh Outdated Show resolved Hide resolved
@c12i
Copy link
Collaborator

c12i commented Jan 16, 2024

@c12i I pushed a commit to hopefully fix the tests. Weird though that github does not show any log output for the failed runs.

I noticed this too, not sure why this is the case. However, I ran the tests locally (with above fixes), and the script exits with this error

holo-dev-server is NOT available in the PATH

@r-vdp
Copy link
Contributor Author

r-vdp commented Jan 16, 2024

@c12i I pushed a commit to hopefully fix the tests. Weird though that github does not show any log output for the failed runs.

I noticed this too, not sure why this is the case. However, I ran the tests locally (with above fixes), and the script exits with this error

holo-dev-server is NOT available in the PATH

They were definitely working locally for me. I'll look into it.

@alastairong alastairong mentioned this pull request Jan 16, 2024
Closed
@c12i c12i mentioned this pull request Jan 17, 2024
Closed
@c12i
Copy link
Collaborator

c12i commented Jan 18, 2024

@r-vdp the tests are still failing after your fix, could you kindly have a look? I would like to support, but I'm unfortunately still new to nix, feel free to follow up with the rest of the team on mattermost for assistance with this if it continues to persist

@r-vdp
Copy link
Contributor Author

r-vdp commented Jan 18, 2024 •
edited
Loading

Hi @c12i. Well, I know why we get the error, but we are still discussing with some other team members about the most desirable manner of fixing it, since there are some trade-offs to take into account.
So I'm waiting for us to take a final decision before continuing on this PR because there's also the possibility that we'll take another approach.

I will mark the PR as draft for now until we've figured out how we want to continue.

For posterity: the current error is because the flake input is pointing to a tar.xz archive and nix is not automatically unpacking that, that's why we then get the error that the source path is not a directory (nix is expecting a directory containing a flake.nix).
I'm not sure why this did work for me locally, it's either because I had this source store path already in my store (but I doubt that), or because this behaviour changed between nix versions (I'm running 2.19.2).

@r-vdp r-vdp marked this pull request as draft January 18, 2024 16:15
@r-vdp r-vdp force-pushed the add_holo_dev_server branch 3 times, most recently from 06dccae to 0c72f4e Compare March 20, 2024 23:06
@r-vdp r-vdp force-pushed the add_holo_dev_server branch from 0c72f4e to b31b462 Compare March 20, 2024 23:07
@r-vdp
Copy link
Contributor Author

r-vdp commented Mar 21, 2024

@c12i we finally got all the required pieces in place at the holo side. Would you be able to give this another look?

@r-vdp
Copy link
Contributor Author

r-vdp commented Mar 21, 2024

Please review the commits separately, as the first two commits are just the result of running standard formatters to avoid reformatting later on.

c12i
c12i approved these changes Mar 21, 2024
View reviewed changes
Copy link
Collaborator

@c12i c12i left a comment

Choose a reason for hiding this comment

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

LGTM, should probably remove it from draft mode, right @r-vdp?

src/cli.rs Show resolved Hide resolved
@r-vdp r-vdp marked this pull request as ready for review March 21, 2024 11:07
@r-vdp
Add holo-dev-server to the dev shell when --holo is passed
a59c2ac 
We add a --holo flag to the example subcommand, it already existed for
the web-app subcommand.
When this flag is passed, we add holo-host/hbs-releases as a flake input
and we add holo-dev-server-bin to the dev shell of the generated nix
flake.
@r-vdp r-vdp force-pushed the add_holo_dev_server branch from b31b462 to a59c2ac Compare March 21, 2024 11:14
@r-vdp
Copy link
Contributor Author

r-vdp commented Mar 21, 2024

@c12i I undrafted and pushed a small change to remove a line from the diff that was not needed anymore.

@c12i c12i merged commit f693a20 into holochain:develop Mar 21, 2024
7 checks passed
@r-vdp r-vdp deleted the add_holo_dev_server branch March 21, 2024 17:42
@c12i c12i added the ShouldBackport/0.2 This change should be backported to develop-0.2 label Apr 8, 2024
c12i added a commit to c12i/scaffolding that referenced this pull request Apr 8, 2024
@c12i
Merge pull request holochain#181 from r-vdp/add_holo_dev_server
3737240
@c12i c12i removed the ShouldBackport/0.2 This change should be backported to develop-0.2 label Apr 8, 2024
c12i added a commit that referenced this pull request Apr 9, 2024
@c12i
chore: Backport and release 0.2 (#258)
8c16312 
* Merge pull request #206 from c12i/ci-improvements

enhancement: improve ci runtime

* Merge pull request #233 from c12i/make-foo-for-bar-components-reactive

fix(templates): Make linked entry-types reactive

* Merge pull request #235 from c12i/bump-ci-actions-versions

chore: bump ci action versions

* Merge pull request #242 from c12i/set-agent-count-via-env-var

feat: add option to set agent count via env var

* Merge pull request #243 from c12i/prefer-dynamic-ports

fix: prefer dynamic ports for starting ui for local development

* Merge pull request #181 from r-vdp/add_holo_dev_server

* fix failing tests

* update app nix flake

* chore: update Cargo lock

* add missng holo flag
@c12i c12i mentioned this pull request Nov 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c12i c12i c12i approved these changes

Assignees

@r-vdp r-vdp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@r-vdp @c12i