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

dotnet watch run shouldn't disable the browser refresh logic injection on Linux #28519

Closed
wants to merge 2 commits into from

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Dec 8, 2020

Prior to this change, inability or failure to launch a browser would result in the browser-refresh feature from being disabled. On Linux the feature is a bit flaky so we always disabled it. This change loosens up some of the restrictions. We always attempt to launch a browser and use heuristics to determine if launching failed. Additionally, the browser refresh feature is enabled regardless of how launching the browser fared.

  • dotnet watch run shouldn't disable the browser refresh logic injection on Linux

Fixes #27081

Description

dotnet watch run relies on OS's file associations to launch the browser the first time it's launched for ASP.NET Core apps. Support for this feature is is a bit iffy on Linux. We took a conservative approach in 5.0 RTM and disabled the ability to both launch the browser and refresh it. We have improved on this and can provide a helpful message when launching a browser did not work, and instruct the user to manually launch it. More importantly, we're able to make the browser refresh feature work all the time since the two are independent.

Customer impact

dotnet watch support on Linux that is more consistent with other platforms.

Regression

No

Risk

Low. The changes are limited to the dotnet-watch tool. We have CTI automation to cover using this and the changes were tested manually on some popular Linux flavors.

@ghost ghost added the area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI label Dec 8, 2020
@pranavkm pranavkm added this to the 5.0.2 milestone Dec 8, 2020
@mkArtakMSFT mkArtakMSFT added the Servicing-consider Shiproom approval is required for the issue label Dec 8, 2020
@ghost
Copy link

ghost commented Dec 8, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

// From emperical observation, it's noted that failing to launch a browser results in either Process.Start returning a null-value
// or for the process to have immediately exited.
// We can use this to provide a helpful message.
_reporter.Output($"Unable to launch the browser. Navigate to {launchUrl}");
Copy link
Member

Choose a reason for hiding this comment

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

If there was an exception, won't they get "Unable to launch browser" twice, does that matter.

Copy link
Contributor

Choose a reason for hiding this comment

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

One is with verbose logging enabled, which isn't common and normally filtered out. @pranavkm maybe change the first log though to remove the duplication?

Copy link
Member

Choose a reason for hiding this comment

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

@pranavkm does this still need to be addressed?

@wtgodbe
Copy link
Member

wtgodbe commented Dec 11, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@wtgodbe
Copy link
Member

wtgodbe commented Dec 11, 2020

@pranavkm are you still trying to get this in for 5.0.2? Today is the cutoff - could you send mail to tactics to get approval?

@wtgodbe wtgodbe modified the milestones: 5.0.2, 5.0.3 Dec 14, 2020
@wtgodbe
Copy link
Member

wtgodbe commented Dec 14, 2020

Moving to 5.0.3 as this didn't get tactics approved in time for 5.0.2

@zbecknell
Copy link
Contributor

@pranavkm This almost fixes #25317 but unfortunately still couples the browser refresh to launchBrowser == true.

Before I saw this PR I created #28885 to decouple that setting with browser refreshing.

When launchBrowser == true we cannot show console colors, so I want to explicitly allow launchBrowser == false while still enabling browser refresh. Can we adjust this PR to accomplish that goal?

@mkArtakMSFT mkArtakMSFT removed the Servicing-consider Shiproom approval is required for the issue label Jan 5, 2021
@mkArtakMSFT
Copy link
Member

I've removed the servicing-consider to avoid this coming up in tactics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants