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

ssh-ng: Only forward settings that make sense to forward. #1946

Closed
wants to merge 1 commit into from

Conversation

shlevy
Copy link
Member

@shlevy shlevy commented Mar 5, 2018

Fixes #1713 and fixes #1935

@shlevy shlevy requested a review from edolstra March 5, 2018 03:15
Copy link
Contributor

@puffnfresh puffnfresh left a comment

Choose a reason for hiding this comment

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

Great stuff

@puffnfresh
Copy link
Contributor

Just pointing out for anybody else; this is not specific to ssh-ng, which is really good for things I've been playing around with.

@edolstra
Copy link
Member

edolstra commented Mar 5, 2018

IMHO this is not the right approach since it will lead to endless confusion and bikeshedding over what settings need to be forwarded. For example, should --cores affect the remote system? Generally not, except when I want to disable parallelism. So it might be better to allow passing arbitrary settings in the store URI (e.g. --store ssh-ng://foo?cores=1).

Regarding build-related flags, polluting config.hh with an ad-hoc flag for whether to forward certain settings seems ugly to me. In fact it's not clear why we can't just use the existing code for forwarding build-related settings, i.e.

    conn.to << wopSetOptions
       << settings.keepFailed
       << settings.keepGoing
    ...

in RemoteStore::setOptions().

@shlevy
Copy link
Member Author

shlevy commented Mar 5, 2018

@edolstra We can't reuse the existing code because it forwards things like keepFailed and buildCores unconditionally. Closing in favor of #1948, which gives us a minimal sane base on top of which we can iterate if need be.

@shlevy shlevy closed this Mar 5, 2018
@shlevy shlevy deleted the forwardable-settings branch March 5, 2018 12:44
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.

Remote builds via ssh-ng try to use local sandbox shell
3 participants