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

feat: Add cargo-doc-live #246

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

leomayleomay
Copy link

@leomayleomay
Copy link
Author

@srid I've got an error when I try to run just test cargo-doc-live

22:28 $ just test cargo-doc-live
nix build ./test#checks.$(nix eval --impure --expr "builtins.currentSystem").cargo-doc-live --override-input services-flake . -L
warning: not writing modified lock file of flake 'git+file:///home/hao/services-flake?dir=test':
• Updated input 'services-flake':
    'github:juspay/services-flake/b23fd8fc01985a994b38acc40bc512abefbbcc3f?narHash=sha256-J0hWG7EAhwujMvhbBfnIc3gMxzPWjj3s5yJNH%2BmV5cs%3D' (2024-06-09)
  → 'git+file:///home/hao/services-flake?ref=refs/heads/add-cargo-doc-live&rev=7e2eb0943215c8d422deb6a3ead4d56e372ee9dd' (2024-06-22)
trace: warning: getExe: Package "browser-sync-3.0.2" does not have the meta.mainProgram attribute. We'll assume that the main program has the same name for now, but this behavior is deprecated, because it leads to surprising errors when the assumption does not hold. If the package has a main program, please set `meta.mainProgram` in its definition to make this warning go away. Otherwise, if the package does not have a main program, or if you don't control its definition, use getExe' to specify the name to the program, such as lib.getExe' foo "bar".
cargo-doc-live-test> + process-compose --no-server -t=false
cargo-doc-live-test> [cargo-doc-live1-cargo-doc ] error: No such file or directory (os error 2)
cargo-doc-live-test> [cargo-doc-live1-cargo-doc ]
cargo-doc-live-test> [cargo-doc-live1-cargo-doc ] error: No such file or directory (os error 2)

{ pkgs, ... }: {
services.cargo-doc-live."cargo-doc-live1" = {
enable = true;
crateName = "simple";
Copy link
Member

@srid srid Jun 22, 2024

Choose a reason for hiding this comment

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

But there's no crate named simple in this PR? This is probably why the PR is failing.

@srid
Copy link
Member

srid commented Jun 22, 2024

Also, the CI never terminates after those error: No such file or directory (os error 2) errors. Can this be fixed so that it terminates immediately?

@leomayleomay leomayleomay marked this pull request as ready for review June 23, 2024 10:26
@leomayleomay leomayleomay requested a review from srid June 23, 2024 10:28
Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

You also want to add the new example to nixci configuration of the top-level flake.nix so it builds in the CI.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a ./example/cargo-doc-live/README.md that explains how to run it?

# `process-compose.foo` will add a flake package output called "foo".
# Therefore, this will add a default package that you can build using
# `nix build` and run using `nix run`.
process-compose."default" = { ... }:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
process-compose."default" = { ... }:
process-compose."cargo-doc-live" = { ... }:

(The default package is usually the Rust package itself)


src = lib.mkOption {
type = types.path;
description = "The src of the cargo";
Copy link
Member

Choose a reason for hiding this comment

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

We need a default value.


src = lib.mkOption {
type = types.path;
description = "The src of the cargo";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description = "The src of the cargo";
description = "Path to the cargo project root";

Comment on lines 32 to 34
default = {
processes =
{
Copy link
Member

Choose a reason for hiding this comment

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

The { in line 34 is indented inconsistently with the rest of the project (e.g.: see { in line 32). You should fix this in similar other places as well.

Comment on lines 40 to 44
let
browser-sync = lib.getExe pkgs.nodePackages.browser-sync;
cargo-watch = lib.getExe pkgs.cargo-watch;
cargo = lib.getExe pkgs.cargo;
in
Copy link
Member

Choose a reason for hiding this comment

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

Since these are in runtimeInputs already, we don't need these and can just reference the executables as is in the script text, no?

runtimeInputs = with pkgs; [ nodePackages.browser-sync ];
text =
''
${lib.getExe pkgs.nodePackages.browser-sync} start --port ${toString config.port} --ss target/doc -s target/doc \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
${lib.getExe pkgs.nodePackages.browser-sync} start --port ${toString config.port} --ss target/doc -s target/doc \
browser-sync start --port ${toString config.port} --ss target/doc -s target/doc \

@@ -0,0 +1,40 @@
# cargo-doc-live

[cargo-doc-live] is live server version of `cargo doc`.
Copy link
Member

Choose a reason for hiding this comment

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

Could you use the 1st paragraph and video from https://github.com/srid/cargo-doc-live in here?


{#crateName}

### The crate to use when opening docs in browser
Copy link
Member

Choose a reason for hiding this comment

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

I know our docs elsewhere are more of skeleton, but we should get into the practice of fleshing out when writing new docs. Can you add some paragraphs to all these sections?

@leomayleomay leomayleomay requested a review from srid June 24, 2024 02:25
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