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

Log ssh output for ssh-ng too #5014

Closed
wants to merge 1 commit into from

Conversation

matthewbauer
Copy link
Member

I think this was just an oversight. Right now, ssh:// gets ssh errors but not ssh-ng://.

/cc @edolstra

I think this was just an oversight. Right now, ssh:// gets ssh errors but not ssh-ng://.

/cc @edolstra
Comment on lines +27 to +30
// Hack for getting remote build log output.
// Intentionally not in `SSHStoreConfig` so that it doesn't appear in
// the documentation
const Setting<int> logFD{(StoreConfig*) this, -1, "log-fd", "file descriptor to which SSH's stderr is connected"};
Copy link
Member

@Ericson2314 Ericson2314 Jul 27, 2021

Choose a reason for hiding this comment

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

N.B. this seems sketch, but is actually fine because this is exactly how LegacySSHStore does it.

@edolstra
Copy link
Member

I think the reason why SSHStore doesn't have this is because it's mostly unnecessary: remote build log output is received via the daemon protocol (as STDERR_* messages). So this will only affect non-daemon error messages like connection issues.

@Ericson2314
Copy link
Member

#6029 will fix this using logs conveyed the way @edolstra described. When that is merged, this can be closed.

FWIW, I think that way is indeed better, because for e.g. a TcpRemoteStore (#5265) we don't easily get an additional stream "for free" anyways.

@Ericson2314
Copy link
Member

Closing as described above.

@Ericson2314 Ericson2314 closed this Mar 7, 2022
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.

4 participants