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

Use ShellExecute to open docs on windows #499

Closed
mattico opened this issue May 25, 2016 · 12 comments · Fixed by #1117
Closed

Use ShellExecute to open docs on windows #499

mattico opened this issue May 25, 2016 · 12 comments · Fixed by #1117

Comments

@mattico
Copy link
Contributor

mattico commented May 25, 2016

rustup 0.1.12 (c6e430a 2016-05-12)
Windows 10 x64 Insider Preview build 14342

Running any of the doc subcommands opens a new command prompt, but does not display the docs. I've tested this from cmd.exe, powershell.exe, and the MSYS2 shell.

Let me know if you need any other info to help debug this issue.

Edit:
I've tried running the command as found in https://github.com/rust-lang-nursery/rustup.rs/blob/master/src/rustup-utils/src/raw.rs#L542, which works fine.

cmd /C start https://github.com

@peschkaj
Copy link
Contributor

Interesting. On Windows 10 (10.0.10586), I see different behavior.

With my default set to either nightly or stable, rustup doc --book first prompted me which program should be used to open the file and then subsequent runs use the browser I chose.

What do you have set in "Default Apps by Protocol"? (Settings -> System -> Default Apps -> Choose default apps by protocol)

@mattico
Copy link
Contributor Author

mattico commented May 25, 2016

@peschkaj I'd assume this would only happen if you don't have a default browser set? Otherwise that is strange. If you run, for example, start chrome https://github.com or start iexplore https://github.com you can specify the browser, FWIW.

@peschkaj
Copy link
Contributor

I would've assumed that I was being prompted because I didn't have a default browser set, too, but this has been my daily driver system since October of last year, so I'm really confused why this particular action bothered to prompt me. 😕

I wonder if you're running into something blocking you on from being on the Insider Preview.

@peschkaj
Copy link
Contributor

I couldn't duplicate this on 10.0.14295, but I'm waiting for the VM to notice that it's on the fast ring.

@luser
Copy link

luser commented May 25, 2016

I just hit this, also running a Windows 10 insider build (14342). I wouldn't be surprised if this is a Windows bug. If I open cmd.exe:

  • start http://google.com works
  • start c:/build/blank.html works
  • start "C:/Users/Ted Mielczarek/.multirust/toolchains/stable-x86_64-pc-windows-msvc/share/doc/rust/html/std/index.html" fails, I get another console window
  • start file:///C:/Users/Ted%20Mielczarek/.multirust/toolchains/stable-x86_64-pc-windows-msvc/share/doc/rust/html/std/index.html works

@mattico
Copy link
Contributor Author

mattico commented May 25, 2016

@luser Interesting. I've submitted it to the insider hub, lets hope they actually see it.

feedback-hub:?contextid=268&feedbackid=b2091da1-3e92-4c8e-bd5c-fadae6f29412&form=1&src=2

Edit: Interestingly, that feedback-hub link also does not work with the start command.

@brson
Copy link
Contributor

brson commented Jun 3, 2016

There must be a smarter way to open the web browser than running cmd.exe.

@luser
Copy link

luser commented Jun 4, 2016

I believe start is just calling ShellExecute under the hood, like: https://support.microsoft.com/en-us/kb/224816

@Diggsey
Copy link
Contributor

Diggsey commented May 4, 2017

Let's re-purpose this issue for the ShellExecute feature. I'm not sure if it will fix the problem, but that seems to be a windows bug, and likely specific to the insider preview.

@Diggsey Diggsey changed the title rustup doc --book fails on Windows Use ShellExecute to open docs on windows May 5, 2017
@Eh2406
Copy link

Eh2406 commented May 12, 2017

I don't see ShellExecute in the winapi 0.2. I don't think @retep998 is taking pr to the 0.2 version any more and It is going to be some time before the 0.3 branch is ready let alone before we switch to it. I'd love to be wrong on any of those points. :-)

@mattico
Copy link
Contributor Author

mattico commented May 12, 2017

This is no longer an issue in W10 build 15063 AKA Creator's Update. I assume that this isn't specific to insider builds (considering that 15063 was an insider build before general availability). It might still be a good idea to use ShellExecute.

We don't necessarily need winapi to use ShellExecute. It's a pretty simple API. We'd just need one extern function, one constant, and make sure that the types of LPCSTR, etc. are correct.

Edit: hm. Unicode doc paths are extremely uncommon, I assume, but this is Rust and we must do the Right Thing:tm:. To support unicode we'd need to use ShellExecuteExW, which is a bit more complicated. Nevermind, just use ShellExecuteW.

@Eh2406
Copy link

Eh2406 commented May 12, 2017

That is a great point. This code worked in one of my projects.

    use winapi;
    extern "system" {
        pub fn ShellExecuteW(
            hwnd: winapi::HWND,
            lpOperation: winapi::LPCWSTR,
            lpFile: winapi::LPCWSTR,
            lpParameters: winapi::LPCWSTR,
            lpDirectory: winapi::LPCWSTR,
            nShowCmd: winapi::c_int,
        ) -> winapi::HINSTANCE;
    }

oops, did not see your edit.

mattico added a commit to mattico/rustup.rs that referenced this issue May 12, 2017
Closes rust-lang#499

The implementation is not too complicated, so it's a bit better than spawning a cmd.exe just to open docs. It also should fix opening docs for the few people still using a year-old insider preview build.
bors added a commit that referenced this issue May 18, 2017
Use ShellExecute rather than start.exe to open docs on windows

Closes #499

The implementation is not too complicated, so it's a bit better than spawning a cmd.exe just to open docs. It also should fix opening docs for the few people still using a year-old insider preview build.
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.

6 participants