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

Workaround to launch URLs #2958

Merged
merged 2 commits into from
Jan 8, 2020
Merged

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Jan 5, 2020

Problem

The homepage and repository links in GUI used to work fine, but recently they stopped working for me. Some investigation revealed that this exception was being thrown:

System.ComponentModel.Win32Exception (0x80004005): Cannot find the specified file
  at System.Diagnostics.Process.StartWithShellExecuteEx (System.Diagnostics.ProcessStartInfo startInfo) [0x00102] in <dfe42095fd31410e95d8021f755c3557>:0 
  at System.Diagnostics.Process.Start () [0x00032] in <dfe42095fd31410e95d8021f755c3557>:0 
  at (wrapper remoting-invoke-with-check) System.Diagnostics.Process.Start()
  at System.Diagnostics.Process.Start (System.Diagnostics.ProcessStartInfo startInfo) [0x0001b] in <dfe42095fd31410e95d8021f755c3557>:0 
  at CKAN.Util.TryOpenWebPage (System.String url, System.Collections.Generic.IEnumerable`1[T] prefixes) [0x0007a] in <5fbaaea77e3f4034bf234bca04213cd3>:0 

Cause

Somehow Process.Start went from launching a URL in the default browser to failing because there isn't a local file named after the URL. The closest thing to an explanation that I was able to find was dotnet/corefx#10361, but I have not been able to confirm it 100%. I tried uninstalling anything that sounded like it was related to corefx, since at some point I had it working fine on Mono, but the problem persists.

Maybe Mono now includes the broken components from corefx? Maybe Mono was updated to be broken in the same way? Maybe corefx is still installed under a package name that I don't recognize? Maybe it's some other totally unrelated problem? Frankly I have no idea, but I'm hoping that someone will explain it in a comment on that issue.

Changes

Now all of our URL launches are done via Utilities.ProcessStartURL, a new function that implements the suggested workaround of launching cmd, open, or xdg-open depending on the platform. This makes it work for me.

While investigating this, I noticed that Util.CheckURLValid rejects HTTPS URLs, which doesn't make sense given that Util.TryOpenWebPage explicitly generates them. Now it's fixed, and Util.TryOpenWebPage tries HTTPS before HTTP since the general trend on the web is to go all-HTTPS.

@HebaruSan HebaruSan added GUI Issues affecting the interactive GUI Core (ckan.dll) Issues affecting the core part of CKAN Pull request Mono Issues specific for Mono ConsoleUI Issues affecting the interactive console UI labels Jan 5, 2020
@HebaruSan HebaruSan requested a review from DasSkelett January 5, 2020 00:43
@DasSkelett
Copy link
Member

DasSkelett commented Jan 7, 2020

Apparently, opening directories doesn't work any more, too (at least on my system):

  1. File > Open KSP directory
  2. CKAN closes.

We also use Process.Start() for this. Debugging shows that it also throws the Win32Exception.

Looks like Mono did a big oopsie with Process.Start()?

@HebaruSan
Copy link
Member Author

HebaruSan commented Jan 7, 2020

Huh, there's mono/mono#17204 which is open and somewhat recent, but it has no discussion or investigation (and it's mislabeled a "proposal" instead of a bug)... 🤦‍♂️

@HebaruSan
Copy link
Member Author

Applied the new workaround to more places in latest changes. Yes, it seems that Process.Start is mostly broken in current Mono.

@DasSkelett
Copy link
Member

Works fine on Linux, but on Windows it launches a console with the URL as title instead of the browser/explorer:
image

@DasSkelett
Copy link
Member

Without enclosing brackets it works.

@HebaruSan
Copy link
Member Author

I wonder if we need the workaround on Windows at all. Maybe the function should just call a normal Process.Start. We could even check Platform.IsMono.

@DasSkelett
Copy link
Member

Current master still works fine on Windows. And if I interpret the issue correctly, it only affects .NET Core on Windows. So the GUI is not a problem, only if we somehow move the console UI to .NET Core in the future we might hit it again.

@HebaruSan
Copy link
Member Author

OK, we'll use the old way for Windows. Until they decide to break that, too, I guess.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Perfect. Whatever Mono does or does not now, we should be safe.

HebaruSan added a commit that referenced this pull request Jan 8, 2020
@HebaruSan HebaruSan merged commit ca149bc into KSP-CKAN:master Jan 8, 2020
@HebaruSan HebaruSan deleted the fix/url-launch branch January 8, 2020 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ConsoleUI Issues affecting the interactive console UI Core (ckan.dll) Issues affecting the core part of CKAN GUI Issues affecting the interactive GUI Mono Issues specific for Mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants