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

cargo doc --open hijacks terminal if browser is not already opened #5701

Closed
Seeker14491 opened this issue Jul 10, 2018 · 12 comments · Fixed by #5888
Closed

cargo doc --open hijacks terminal if browser is not already opened #5701

Seeker14491 opened this issue Jul 10, 2018 · 12 comments · Fixed by #5888

Comments

@Seeker14491
Copy link
Contributor

When the command cargo doc --open is invoked while the user's browser is not open, the browser is started in a non-detached manner, and the user's terminal blocks until the browser is closed. This is in contrast to running the same cargo doc --open command with a browser already open, in which case the terminal does not block.

This discrepancy is confusing, and having your terminal blocked is annoying, as you're now forced to either open another terminal instance or close the docs before you can input any commands. Therefore I propose cargo doc --open never block the user's terminal.

I tested on Windows 10 and Linux, and observed the same behavior on both platforms.

@dwijnand
Copy link
Member

I couldn't reproduce this with macOS and Chrome. Whether my browser is already opened or not I get the docs and an unblocked terminal:

09:03:29 $ cargo doc --open
    Finished dev [unoptimized + debuginfo] target(s) in 0.24s
     Opening /d/cargo/target/doc/cargo/index.html
   Launching open

09:03:30 $

@Seeker14491
Copy link
Contributor Author

Looking at the code that handles opening the docs, different code is executed for Windows, Mac, and other platforms, which explains why Mac isn't affected.

@Seeker14491
Copy link
Contributor Author

I've got a fix for Windows; I'll look into fixing it on Linux and then send a pull request.

@dwijnand
Copy link
Member

Nice one. Thanks @Seeker14491! (I was going to suggest copying what rustup uses)

@Seeker14491
Copy link
Contributor Author

@dwijnand great idea about looking at rustup. Indeed, the rustup doc command doesn't have this bug. This is the code that handles opening a path in the browser in rustup: link. The Windows code there takes quite a different approach, calling a Windows API function. I'm not sure what the benefit is over the simpler approach taken by cargo. The fix I implemented was simply replacing cmd /C with explorer in the Windows code.

@Seeker14491
Copy link
Contributor Author

Seeker14491 commented Jul 30, 2018

This rustup commit contains justification for going the Windows API route, so I suppose we should do the same. I don't like the idea of just copy-pasting the code though; I think it would be better if cargo and rustup used the same code to handle this by pulling in a crate like webbrowser. webbrowser doesn't use the Windows API method though, so I'd want to change it so that it does before we use it. Does this sound like a good idea?

edit: There is also the open crate.

@dwijnand
Copy link
Member

WDYT @alexcrichton and @Diggsey? Should we lift what Rustup does into a crate and reuse it here?

@Diggsey
Copy link
Contributor

Diggsey commented Jul 31, 2018

I'd be happy to accept a PR to rustup pulling this functionality out into a crate, assuming it is at least as functional.

@alexcrichton
Copy link
Member

Seems reasonable to me!

@Seeker14491
Copy link
Contributor Author

I've published the opener crate. I've run into a problem when integrating it into cargo though, namely, the browser will not open at all on Windows! I believe this is due to Cargo's Windows-specific handling of killing its child processes when it exits, an issue similar to what's happening in #5598. If I add in a 10 second sleep after calling the open doc function, the browser opens, but then abruptly closes after the 10 seconds.

@alexcrichton
Copy link
Member

@Seeker14491 oh that's #5598 I believe which I believe we can indeed fix!

Seeker14491 added a commit to Seeker14491/cargo that referenced this issue Aug 14, 2018
bors added a commit that referenced this issue Aug 15, 2018
Handle opening browser with `opener` crate

Fixes #5701

A note about behavior on Linux:
When looking for a browser to open the docs with, Cargo currently tries the `$BROWSER` environment variable, `xdg-open`, `gnome-open`, and finally `kde-open`, in that order. With this commit, this behavior changes; the `opener` crate always uses the `xdg-open` script, which is included in the `opener` crate. The `xdg-open` script takes care of finding a suitable browser.
@siddharthgpta
Copy link

I don't know if this is being tracked in a separate issue, but I see this behavior on my MacBook when running:
BROWSER=/Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome cargo doc --open

MacOS: 12.2.1 (21D62)
Model: MBP 16" M1 Pro
Cargo: 1.59.0 (49d8809dc 2022-02-10)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants