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

Add --host option to docc preview #713

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

Conversation

tomstuart
Copy link

Summary

I installed Swift-DocC on a remote server and wanted to run docc preview to preview my docs with my local browser, but it didn’t work because the preview server binds to the loopback interface (localhost) without any option to receive connections from other hosts.

This PR adds a --host option so that the preview server can be bound to an address on a real network interface (e.g. the machine’s IP address, or just 0.0.0.0 to bind to all IPv4 interfaces) to allow other hosts to connect.

Testing

Steps:

  1. Run swift run docc preview as described in CONTRIBUTING.md
  2. On another machine, try visiting http://<server-address>:8080/documentation/swiftdocc in a browser, which will fail to connect
  3. Stop the preview server and run swift run docc preview again, this time providing the --host 0.0.0.0 option
  4. Visit http://<server-address>:8080/documentation/swiftdocc again and see it work

Checklist

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

Although I’ve updated the existing tests to support this change, I can’t see any tests which actually exercise the network behaviour of docc preview, so I didn’t feel comfortable introducing one here.

@tomstuart
Copy link
Author

Once this is merged I intend to open another PR to rename the enumeration cases from localhost and socket to something more descriptive, e.g. ip and unix, to make it clearer that the IP server is no longer necessarily bound to the loopback interface.

@@ -201,7 +201,7 @@ class PreviewServerTests {
}

func testPreviewServerBindDescription() {
let localhostBind = PreviewServer.Bind.localhost(port: 1234)
let localhostBind = PreviewServer.Bind.localhost(host: "localhost", port: 1234)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also test that by default, "localhost" is the host?

Copy link
Author

Choose a reason for hiding this comment

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

It’s the default in the sense that the --host option (in PreviewOptions) now has the value localhost by default, but PreviewServer itself has no default host — an explicit value is always required. Are you suggesting we add a PreviewOptions test to check that --host gets the right default, or something different? There’s not currently a PreviewOptionsTests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it would be good to test that --host gets the right default. The best place for that would be in PreviewSubcommandTests, it looks like.

@@ -84,6 +86,7 @@ public final class PreviewAction: Action, RecreatingContext {
/// is performed.
/// - Throws: If an error is encountered while initializing the documentation context.
public init(
host: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a public initializer, providing a default value here would avoid a source breaking change for anyone who imports SourceDocCUtilities.

@tomstuart tomstuart force-pushed the preview-server-host-option branch 2 times, most recently from 780cc18 to a500b6b Compare September 27, 2023 14:46
`ServerBootstrap.bind` needs to know which host name to bind a socket
to, but we were hardcoding it to `localhost` without providing a way for
a different host to be provided. By adding a `host:` element to
`PreviewServer.Bind.localhost`, we require the caller of
`PreviewServer.init` to be specific about which host they want.
If there’s a problem binding to a local port, the specific host name
might be relevant.
Now that `PreviewServer` requires an explicit host name, we should
require one here too instead of hardcoding `localhost`.
This retains the `localhost` default host name for the preview server,
but allows the user to override it by providing `--host`.

We’re only supporting the long `--host` option because `-h` already
means `--help`.
Because `localhost` only allows connections on the loopback interface,
anybody who’s trying to run `docc preview` on a remote server and
connect to it with their local browser will need to use `--host 0.0.0.0`
or similar.
Copy link
Contributor

@Kyle-Ye Kyle-Ye left a comment

Choose a reason for hiding this comment

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

I'm generally -0.5 for this PR.

  1. IMO, Preview is actually a little out of scope of Swift-DocC. So we only provide basic support for it(Just my opinion).
    If people want advanced feature for the preview(eg. check the preview on other network device), they can start a http server manually. It's very easy for almost every OS and in all language. We should not add complex here on DocC side.

  2. The current API design of PreviewAction is public. So we must preserve the old API to maintain source compatibility. It's actually a bit complex.

If other DocC folks agrees the idea of the PR. You can have a look at #254 for reference to fix the compatibility issue.

@@ -108,13 +112,14 @@ public final class PreviewAction: Action, RecreatingContext {
@available(*, deprecated, message: "TLS support has been removed.")
public convenience init(
tlsCertificateKey: URL?, tlsCertificateChain: URL?, serverUsername: String?,
serverPassword: String?, port: Int,
serverPassword: String?, host: String, port: Int,
Copy link
Contributor

Choose a reason for hiding this comment

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

This initializer is already deprecated, we should not change the signature

@@ -84,6 +86,7 @@ public final class PreviewAction: Action, RecreatingContext {
/// is performed.
/// - Throws: If an error is encountered while initializing the documentation context.
public init(
host: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. duplicate the original init and add host parameter.
  2. mark the original init as deprecate.

Copy link
Contributor

@d-ronnqvist d-ronnqvist Oct 4, 2023

Choose a reason for hiding this comment

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

Like I commented here I think it's sufficient to provide a default value for this added parameter to remain source compatible. It won't be ABI compatible but we don't require that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. Ethan's PR #254 is removing parameters. So he deprecated the old and add a new one.
For this PR, this is just an addition. If we only cares about source compatible, it is indeed enough to provide a default value.

@tomstuart
Copy link
Author

If people want advanced feature for the preview(eg. check the preview on other network device), they can start a http server manually. It's very easy for almost every OS and in all language. We should not add complex here on DocC side.

I don’t disagree, but (for whatever reason) we already provide --port as a way to override the default value for one of ServerBootstrap.bind’s arguments; my intention here is to provide --host as a way to override the other one.

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Oct 4, 2023

but (for whatever reason) we already provide --port as a way to override the default value for one of ServerBootstrap.bind’s arguments;

The --port support was added when swift-docc is open sourced for the first commit. Maybe @d-ronnqvist know the context here.
The port used to default to 8000. And it changed to 8080 on #373

my intention here is to provide --host as a way to override the other one.

Got it. Make sense for me.

@d-ronnqvist
Copy link
Contributor

but (for whatever reason) we already provide --port as a way to override the default value for one of ServerBootstrap.bind’s arguments;

The --port support was added when swift-docc is open sourced for the first commit. Maybe @d-ronnqvist know the context here. The port used to default to 8000. And it changed to 8080 on #373

my intention here is to provide --host as a way to override the other one.

Got it. Make sense for me.

I feel that it's fine to add a --host option to the preview command when we already have a --port option.

It's been several years so I don't remember any particular context around when we added the --port option. It may have "always" been there.

@d-ronnqvist d-ronnqvist added the source breaking DocC's public API isn't source compatible with earlier versions label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
source breaking DocC's public API isn't source compatible with earlier versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants