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

Renamed ping to info in store #8975

Closed
wants to merge 13 commits into from
+17 −16
Closed

Renamed ping to info in store #8975

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5cb0bf8
ping renamed to info
vicky1999 Sep 13, 2023
ecde425
testcases updated for store ping
vicky1999 Sep 13, 2023
17619b2
formatting changes
vicky1999 Sep 13, 2023
6973044
test file renamed
vicky1999 Sep 13, 2023
91b314e
reverted ping store alias
vicky1999 Sep 13, 2023
8f4557b
store info files renamed
vicky1999 Sep 13, 2023
1a4f4f9
changes to display trusted as bool instead of number
vicky1999 Sep 13, 2023
7fcc122
info store command formatting fixes
vicky1999 Sep 13, 2023
0333d18
formatting chagnes to resolve doc error in store info
vicky1999 Sep 13, 2023
0652e26
store info json display will be indented
vicky1999 Sep 14, 2023
fc094c1
alias implementation in args
vicky1999 Sep 18, 2023
0556de9
comments added
vicky1999 Sep 18, 2023
7fd7db7
reverted aliasing in args.cc and json formatting for store info
vicky1999 Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/nix/ping-store.cc → src/nix/info-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@ struct CmdPingStore : StoreCommand, MixJSON

std::string doc() override
{
return
#include "ping-store.md"
;
return #include "info-store.md"
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal (and no blocker to me), but since we're renaming things, we could rename this to store-info.md to match the command (ping-store comes from the original command name and was already out of sync)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

store-info files are renamed according to the command name

Copy link
Member

Choose a reason for hiding this comment

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

I think what @thufschmitt is saying is that it would be good to call both files store-info.{cc,md}. The file was never properly renamed when the commands were made hierarchical before, but we should fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will try to implement the aliasing in store.cc. Thanks

;
}

void run(ref<Store> store) override
Expand Down Expand Up @@ -46,4 +45,4 @@ struct CmdPingStore : StoreCommand, MixJSON
}
};

static auto rCmdPingStore = registerCommand2<CmdPingStore>({"store", "ping"});
static auto rCmdPingStore = registerCommand2<CmdPingStore>({"store", "info"});
14 changes: 7 additions & 7 deletions src/nix/ping-store.md → src/nix/info-store.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,28 @@ R""(

# Examples

* Test whether connecting to a remote Nix store via SSH works:
- Test whether connecting to a remote Nix store via SSH works:

```console
# nix store ping --store ssh://mac1
# nix store info --store ssh://mac1
```

* Test whether a URL is a valid binary cache:
- Test whether a URL is a valid binary cache:

```console
# nix store ping --store https://cache.nixos.org
# nix store info --store https://cache.nixos.org
```

* Test whether the Nix daemon is up and running:
- Test whether the Nix daemon is up and running:

```console
# nix store ping --store daemon
# nix store info --store daemon
```

# Description

This command tests whether a particular Nix store (specified by the
argument `--store` *url*) can be accessed. What this means is
argument `--store` _url_) can be accessed. What this means is
dependent on the type of the store. For instance, for an SSH store it
means that Nix can connect to the specified machine.

Expand Down
2 changes: 1 addition & 1 deletion src/nix/main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ struct NixArgs : virtual MultiCommand, virtual MixCommonArgs
{"ls-store", {"store", "ls"}},
{"make-content-addressable", {"store", "make-content-addressed"}},
{"optimise-store", {"store", "optimise"}},
{"ping-store", {"store", "ping"}},
{"info-store", {"store", "info"}},
Copy link
Member

Choose a reason for hiding this comment

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

That one shouldn't be changed. The ping-store alias should stay the same.
We should also have a backwards-compat alias from store ping to store info I think. @Ericson2314 @edolstra is there a mechanism for aliasing subcommands? If not we can always do it manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. So, According to the comment, the ping-store need to stay the same but, the vector alias of it {"store", "info"} can be as it is. please let me know if this is the expectation

Copy link
Member

Choose a reason for hiding this comment

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

I do not off-hand know of a mechanism for aliasing within subcommands, but if we look how this is implemented with overriding rewriteArgs, perhaps that can be done within the Store command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the store command in store-info.cc. After that, I had a suggestion that, if we are renaming the command from ping to info, then I thought it would be better to also update the alias accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure what you are saying? I am saying you can modify src/nix/store.cc to that nix store ping is an alias for nix store info in addition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please let me know if there are any documentations available for this aliasing.

Copy link
Member

Choose a reason for hiding this comment

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

There is no documentation for this exact task. You would need to figure out what main.cc is doing and recreate it in store.cc. I am sorry.

It would be good to add API docs to rewriteArgs where it is declared (args.hh). Perhaps once you've figured out what is going on your can do that, so the next person is less stuck?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will try to implement it in store.cc. Thanks.

{"sign-paths", {"store", "sign"}},
{"show-derivation", {"derivation", "show"}},
{"to-base16", {"hash", "to-base16"}},
Expand Down
8 changes: 4 additions & 4 deletions tests/store-ping.sh → tests/store-info.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
source common.sh

STORE_INFO=$(nix store ping 2>&1)
STORE_INFO_JSON=$(nix store ping --json)
STORE_INFO=$(nix store info 2>&1)
STORE_INFO_JSON=$(nix store info --json)

echo "$STORE_INFO" | grep "Store URL: ${NIX_REMOTE}"

Expand All @@ -11,7 +11,7 @@ if [[ -v NIX_DAEMON_PACKAGE ]] && isDaemonNewer "2.7.0pre20220126"; then
[[ "$(echo "$STORE_INFO_JSON" | jq -r ".version")" == "$DAEMON_VERSION" ]]
fi

expect 127 NIX_REMOTE=unix:$PWD/store nix store ping || \
fail "nix store ping on a non-existent store should fail"
expect 127 NIX_REMOTE=unix:$PWD/store nix store info || \
fail "nix store info on a non-existent store should fail"

[[ "$(echo "$STORE_INFO_JSON" | jq -r ".url")" == "${NIX_REMOTE:-local}" ]]