Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Support TildeSlash expand #8589

Merged
merged 1 commit into from
Feb 12, 2018
Merged

Conversation

wli3
Copy link

@wli3 wli3 commented Feb 10, 2018

Customer scenario

Consumer install a global tool on CLI that has the command name dotnet-mytool. The consumer cannot invoke the command by dotnet mytool.

Bugs this fixes

https://github.com/dotnet/cli/issues/8450

Workarounds, if any

Add the full path of shim to PATH manually.

Risk

low

Performance impact

no

Root cause analysis

CLI command resolver does not implementation the same path logic as bash.

How was the bug found?

Issue filed on GitHub


fix https://github.com/dotnet/cli/issues/8450

Tilda expand is a bash behavior. I only add "~/" expand to $HOME

I cannot find a good way to test PATH. Considering we should not put things in CI's home directory. And if we mock a little bit, we are testing string class. So I only manually tested it.

@wli3 wli3 requested a review from a team February 10, 2018 03:21
private string ExpandTildeSlash(string path)
{
string tildeSlash = "~/";
if (path.StartsWith(tildeSlash) && !string.IsNullOrEmpty(_userHomeDirectory.Value))

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@livarcocc livarcocc added this to the 2.1.3xx milestone Feb 11, 2018
@@ -54,6 +56,19 @@ private IEnumerable<string> SearchPaths
}
}

private string ExpandTildeSlash(string path)
{
string tildeSlash = "~/";

This comment was marked as spam.

@wli3 wli3 force-pushed the fix-donet-x-no-on-mac branch 2 times, most recently from 64bfb5b to c1def3d Compare February 12, 2018 21:05
@wli3 wli3 changed the base branch from master to release/2.1.3xx February 12, 2018 21:05
@wli3
Copy link
Author

wli3 commented Feb 12, 2018

@MattGertz for approval
@livarcocc retargeted to 2.1.3xx and added const

private string ExpandTildeSlash(string path)
{
const string tildeSlash = "~/";
if (path.StartsWith(tildeSlash) && !string.IsNullOrEmpty(_userHomeDirectory.Value))

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@tannergooding
Copy link
Member

Why are we only handling "~/"? The bug only lists that one, but it can be the case that any of them impacts our search logic...

This seems like something there should be a more general .NET API for resolving...

@wli3
Copy link
Author

wli3 commented Feb 12, 2018

Why are we only handling "~/"? The bug only lists that one, but it can be the case that any of them impacts our search logic...

This seems like something there should be a more general .NET API for resolving...

I agree @tannergooding . Path.Get full path should do the work. Or path.GetFullPathAsBash

@nguerrera
Copy link
Contributor

I agree @tannergooding . Path.Get full path should do the work. Or path.GetFullPathAsBash

cc @JeremyKuhne Has this come up as a feature request on corefx before. Should we file something?

@wli3 wli3 merged commit 2a493c1 into dotnet:release/2.1.3xx Feb 12, 2018
@wli3 wli3 deleted the fix-donet-x-no-on-mac branch February 12, 2018 22:59
@JeremyKuhne
Copy link
Member

I don't think anyone has asked to have a separate API. That is worth considering. Possibly we could add options? There are other things folks might want to tweak, such as 8.3 expansion.

livarcocc added a commit that referenced this pull request Feb 15, 2018
…ease/2.1.3xx-to-master

* dotnet/release/2.1.3xx:
  Update to aspnetcore 2.1.0-preview1-28275 and react to feed layout changes (#8611)
  "ExternalRestoreSources" needs to be set in the docker container (#8602)
  Signing nupkg contents (Cli.Utils and MSBuildResolver) along with the rest of the compiled assemblies.
  Use satellites from roslyn package, not cli-deps-satellites
  Update to roslyn 2.7.0-beta3-62612-07 for 2.1.1xx
  Support TildeSlash expand (#8589)
  Port Kernel Version telemetry to preview1
  Do not create a directory with a trailing space; it cannot be deleted by conventional methods. (#8587)
  Consume generic aspnetcore rpm installers
  Insert NuGet Build 4.6.0-rtm-4918 into cli
  Adding roslyn to automatic dependency flow through maestro.
  Fixing update dependency by using the new APIs. We broke this when we updated the version of VersionTools.
  MSBuild 15.6.81
  Update SDK to 2.1.300-preview1-62608-07
  MSBuild 15.6.80

 Conflicts:
	build/DependencyVersions.props
	test/Microsoft.DotNet.ShellShim.Tests/ShellShimMakerTests.cs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants