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

cli: allow to specify only the --rpc-file option #3353

Merged
merged 1 commit into from
Dec 26, 2019
Merged

cli: allow to specify only the --rpc-file option #3353

merged 1 commit into from
Dec 26, 2019

Conversation

darosior
Copy link
Collaborator

fixes #3352

I at first made the --rpc-file option take over the --lightning_dir one, but we don't seem to be using the config dir in the cli anyway, so ..

shesek added a commit to ElementsProject/lightning-charge that referenced this pull request Dec 17, 2019
@shesek
Update testenv instructions for c-lightning v0.8.0
7b32547 
This won't actually work without ElementsProject/lightning#3353,
but neither will the instructions currently printed out (LN_ALICE_PATH and
LN_BOB_PATH already include the network subdirectory and `--network` is not
specified), so probably better that way anyway.
ZmnSCPxj
ZmnSCPxj approved these changes Dec 18, 2019
View reviewed changes
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

ACK 8482a65

Solution seems fine, @shesek could you confirm?

@shesek
Copy link
Contributor

shesek commented Dec 18, 2019 •
edited
Loading

This did resolve #3352, but it looks like the chdir was needed to resolve the path of lightning-rpc as relative to the data dir. Without it, c-lightning attempts to connect to lightning-rpc relative to the CWD and not to <lightning-dir>/<network>/lightning-rpc.

Edit: if you're looking for a v0.8.0 workaround, see this.

@cdecker
Copy link
Member

cdecker commented Dec 18, 2019

Notice also that the chdir papered over another quirk of AF_UNIX sockets: the path length is limited to ~103 characters. I ran into this with the testing framework which can't chdir and the paths are generated from the test names (sometimes resulting in longer paths). So if chdir is removed here, we might expose ourselves to the same issue.

@darosior
Copy link
Collaborator Author

darosior commented Dec 18, 2019 •
edited
Loading

Yeah the default is relative but I tested using an absolute path, will correct to handle both cases.

we might expose ourselves to the same issue.

Ok I'll definitely leave the chdir.

@darosior
Copy link
Collaborator Author

So this is now the --rpc-file option taking over --lightning-dir and --network (and thus don't need them) if specified as an absolute path.

@shesek
Copy link
Contributor

shesek commented Dec 18, 2019

Tested ACK 1b5cb2f, works as expected for both absolute and relative --rpc-file locations.

@cdecker
Copy link
Member

cdecker commented Dec 18, 2019

The CI failures seem unrelated, working on getting those fixed first. Other than that this seems good to go 👍

ACK 1b5cb2f

@shesek shesek mentioned this pull request Dec 18, 2019
Closed
@darosior darosior changed the title cli: don't chdir into lightning-dir cli: allow to specify only the --rpc-file option Dec 19, 2019
@jb55
Copy link
Collaborator

jb55 commented Dec 23, 2019 via email

@darosior
cli: make '--rpc-file' take over other config options if absolute
1a82e5a 
If absolute, we can deduce the lightning_dir, the network, and the
socket filename out of it.
This allows to be able to only specify the 'rpc-file', as before.

Changelog-Changed: Usage of `lightning-cli` by specifying only `--rpc-file` has been restored.
@darosior
Copy link
Collaborator Author

Fixed, thanks !

@ZmnSCPxj
Copy link
Collaborator

ACK 1a82e5a

@ZmnSCPxj ZmnSCPxj merged commit 4f3e8d4 into ElementsProject:master Dec 26, 2019
@darosior darosior deleted the fix_cli branch December 26, 2019 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZmnSCPxj ZmnSCPxj ZmnSCPxj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issuing RPC commands to a specified rpc socket file without knowing its network
5 participants
@darosior @shesek @cdecker @jb55 @ZmnSCPxj