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

relative/absolute path issues in shim after installing donet tool #9414

Closed
csMACnz opened this issue May 20, 2018 · 22 comments
Closed

relative/absolute path issues in shim after installing donet tool #9414

csMACnz opened this issue May 20, 2018 · 22 comments
Assignees
Labels
Milestone

Comments

@csMACnz
Copy link
Contributor

csMACnz commented May 20, 2018

After installing dotnet-sdk-2.1.300-rc1-008673 on Linux, Installed packages using --tool-path produces a shim that cannot locate the actual tool dll correctly.

Steps to reproduce

sudo apt-get install dotnet-sdk-2.1.300-rc1-008673 -y
dotnet tool install coveralls.net --version 1.0.0-beta0003 --tool-path tools
.\tools\csmacnz.Coveralls --help

Example Results on appveyor:

Expected behavior

The application dll executes via the shim application (equivalent to dotnet csmacnz.Coveralls.dll)

Actual behavior

The shim trys to use the wrong path on linux

The application to execute does not exist: '/home/appveyor/projects/dotnettoolsbugrepro/tools/..//home/appveyor/projects/dotnettoolsbugrepro/tools/.store/coveralls.net/1.0.0-beta0003/coveralls.net/1.0.0-beta0003/tools/netcoreapp2.1/any/csmacnz.Coveralls.dll'.
162
Command exited with code 154

This path looks like it is incorrectly appending an absolute path in place of a relative one. (I don't know about the ../ part though.)

Broken build example here

I have also verified that /home/appveyor/projects/dotnettoolsbugrepro/tools/.store/coveralls.net/1.0.0-beta0003/coveralls.net/1.0.0-beta0003/tools/netcoreapp2.1/any/csmacnz.Coveralls.dll does exist.

Note also that this worked fine on build dotnet-sdk-2.1.300-preview2-008533 on Linux (see Previous passing build in test repo) and this also works with the rc1 version running under windows

Environment data

dotnet --info output:

.NET Core SDK (reflecting any global.json):
 Version:   2.1.300-rc1-008673
 Commit:    f5e3ddbe73

Runtime Environment:
 OS Name:     ubuntu
 OS Version:  16.04
 OS Platform: Linux
 RID:         ubuntu.16.04-x64
 Base Path:   /usr/share/dotnet/sdk/2.1.300-rc1-008673/
Host (useful for support):
  Version: 2.1.0-rc1
  Commit:  eb9bc92051

.NET Core SDKs installed:
  1.1.5 [/usr/share/dotnet/sdk]
  1.1.6 [/usr/share/dotnet/sdk]
  1.1.7 [/usr/share/dotnet/sdk]
  2.0.0 [/usr/share/dotnet/sdk]
  2.0.2 [/usr/share/dotnet/sdk]
  2.0.3 [/usr/share/dotnet/sdk]
  2.1.2 [/usr/share/dotnet/sdk]
  2.1.3 [/usr/share/dotnet/sdk]
  2.1.4 [/usr/share/dotnet/sdk]
  2.1.101 [/usr/share/dotnet/sdk]
  2.1.300-rc1-008673 [/usr/share/dotnet/sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.0-rc1-final [/usr/share/dotnet/shared/Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.0-rc1-final [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 1.0.8 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 1.0.9 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 1.1.5 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 1.1.6 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.0 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.3 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.4 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.5 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.6 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.0-rc1 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download
@livarcocc
Copy link
Contributor

@peterhuene @wli3

@peterhuene peterhuene self-assigned this May 21, 2018
@peterhuene
Copy link
Contributor

I'll investigate this immediately.

@wli3
Copy link

wli3 commented May 21, 2018

I have a repo. Use full path as workaround can mitigate it for now.

dotnet tool install coveralls.net --version 1.0.0-beta0003 --tool-path /home/wul/tools

@wli3
Copy link

wli3 commented May 21, 2018

digging why

@wli3
Copy link

wli3 commented May 21, 2018

probably something wrong here https://github.com/dotnet/cli/blob/3fe29161af45ccf76e891e0ceee486176561dc6c/src/Microsoft.DotNet.Cli.Utils/PathUtility.cs#L108

@peterhuene
Copy link
Contributor

I was also able to reproduce and I suspect you're right that something is tripping up GetRelativePath. Assigning this over to you for now.

@wli3
Copy link

wli3 commented May 21, 2018

@csMACnz

 dotnet tool install coveralls.net --version 1.0.0-beta0003 --tool-path $(pwd)/tool

Should unblock you for now

@csMACnz
Copy link
Contributor Author

csMACnz commented May 21, 2018

Thanks for the workarounds 👍

@peterhuene
Copy link
Contributor

peterhuene commented May 21, 2018

@wli3 it appears the shim path passed to PathUtility.GetRelativePath is still relative to CWD and it doesn't handle that well. Either PathUtility.GetRelativePath should make the first argument a full path (a scarier change) or AppHostShellShimMaker should make it a full path before calling PathUtility.GetRelativePath.

@wli3
Copy link

wli3 commented May 21, 2018

@peterhuene i see. I am not sure corefx GetRelativePath is available yet. By the time it was not. https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/tests/System/IO/Path.GetRelativePath.cs#L78

I should add more tests before using a util i find deep in our code base.

@peterhuene
Copy link
Contributor

peterhuene commented May 21, 2018

We should also be using Path.GetRelativePath instead of PathUtility. It should be available (just tested).

I've also tested that Path.GetRelativePath handles the shim path being relative to CWD correctly.

@peterhuene
Copy link
Contributor

peterhuene commented May 21, 2018

AppHostShellShimMaker appears to be the only non-test location calling PathUtility.GetRelativePath, so I think we can remove this implementation, or at least move it to the tests.

@wli3
Copy link

wli3 commented May 21, 2018

did Path.GetRelativePath flowed to cli?

@wli3
Copy link

wli3 commented May 21, 2018

it is for 2.1.4xx . i guess this will goes to 2.1.4xx. So this should be fine

@peterhuene
Copy link
Contributor

peterhuene commented May 21, 2018

I just changed AppHostShellShimMaker to call Path.GetRelativePath. We can't use Path.GetRelativePath in the util assembly because it targets netstandard 2.0.

@wli3
Copy link

wli3 commented May 21, 2018

There is no test covering relative path in Path.GetRelativePath https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/tests/System/IO/Path.GetRelativePath.cs#L78

Maybe we need to getfullpath anyway

@peterhuene
Copy link
Contributor

peterhuene commented May 21, 2018

@livarcocc @KathleenDollard I think we'll need an RTM known issue entry for this.

If users specify a relative tool path on Linux and macOS, the generated shim will not work properly. The workaround is to specify a full path to --tool-path. The test we added when the --tool-path option was regressed for relative paths previously only ensures the tool successfully installed; the new issue is that the tool installs successfully, but the shim generated has an invalid path embedded. The apphost change revealed an existing bug in our path utility class responsible for making relative paths.

@peterhuene
Copy link
Contributor

I'm fine with Path.GetRelativePath(Path.GetFullPath(appHostDestinationFilePath), entryPoint.Value) just in case. We've already resolved the entry point path to the full path I believe.

@wli3
Copy link

wli3 commented May 21, 2018

And according the comment on GetRelativePath, the first argument is a directory. We need to make a getdirectory call. Let's make all of them fullpath. Just to be safe.

I should play around Path.GetRelativePath for a while. To make sure I do not assume more.

@wli3
Copy link

wli3 commented May 21, 2018

And after I resolve this immediate issue. I need to make DirectoryPath and FilePath take a DI Path class. And always store FullPath in these 2 classes.(without DI Path class, test can only run on Windows or Unix) These 2 are almost functions. But they are actually not. Path still depends heavily on filesystem (/home means different stuff on windows and unix.) And put Path into mock file system like IFile. This should also resolve the weird test issue on mock file system.

Ideally we should abstract Path away. But we have so many operation rely on specific file layout. I don't have a better solution for it

@wli3
Copy link

wli3 commented May 21, 2018

BTW no repo on Windows

@peterhuene
Copy link
Contributor

Closed as fixed with dotnet/cli#9330. Thanks again @csMACnz for reporting this issue for us! It made it a little too late to get it into the RTM of 2.1.300 (the upcoming .NET Core 2.1 release), but we hope to port this fix from release/2.1.4xx into a service release for 2.1.3xx. For now, the workaround is to use an absolute path for the --tool-path option on macOS and Linux.

@livarcocc do we have a place to record 2.1.3xx servicing fixes?

wli3 referenced this issue in wli3/cli May 30, 2018
There is no need to store relative path today. But some part of the system does not accept relative path and there is no indication if it is storing full path or not. This is the root cause of https://github.com/dotnet/cli/issues/9319

“someplace” means different full path for Path class on unix and Windows. And the mock file system uses real Path class. Change tests' setup to use essentially “TEMPATH/someplace” instead of  “someplace”
wli3 referenced this issue in dotnet/cli Jun 6, 2018
There is no need to store relative path today. But some part of the system does not accept relative path and there is no indication if it is storing full path or not. This is the root cause of https://github.com/dotnet/cli/issues/9319

“someplace” means different full path for Path class on unix and Windows. And the mock file system uses real Path class. Change tests' setup to use essentially “TEMPATH/someplace” instead of  “someplace”
Mizux referenced this issue in google/or-tools Jul 18, 2018
- On netcoreapp2.0 seems to have some issue with the "current working dir".
@msftgits msftgits transferred this issue from dotnet/cli Jan 31, 2020
@msftgits msftgits added this to the 2.1.4xx milestone Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants