Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Multilevel SDK Lookup#1568

Merged
gkhanna79 merged 2 commits into
dotnet:masterfrom
cakine:sdkLookup
Feb 22, 2017
Merged

Multilevel SDK Lookup#1568
gkhanna79 merged 2 commits into
dotnet:masterfrom
cakine:sdkLookup

Conversation

@cakine
Copy link
Copy Markdown

@cakine cakine commented Feb 21, 2017

.NETCore activation will look for the SDK version folder in the current working directory, the user location, the executable directory and the global .NET location, in the order described.

The user and global locations are defined as follows:

User location:
Windows 32-bit: %SystemDrive%\Users\username.dotnet\x86
Windows 64-bit: %SystemDrive%\Users\username.dotnet\x64
Unix 32-bit: /home/username/.dotnet/x86
Unix 64-bit: /home/username/.dotnet/x64

Global location:
Windows 32-bit: %SystemDrive%\Program Files\dotnet
Windows 64-bit (32-bit application): %SystemDrive%\Program Files (x86)\dotnet
Windows 64-bit (64-bit application): %SystemDrive%\Program Files\dotnet
Unix: the directory of “dotnet” defined in the system path.

@dnfclas
Copy link
Copy Markdown

dnfclas commented Feb 21, 2017

Hi @cakine, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@cakine
Copy link
Copy Markdown
Author

cakine commented Feb 21, 2017

@gkhanna79 @ramarag

@cakine cakine changed the title SDK Multilevel Lookup Multilevel SDK Lookup Feb 21, 2017
Comment thread src/corehost/cli/fxr/fx_muxer.cpp Outdated
pal::string_t env_lookup;

// Multi-level SDK lookup can be disabled by setting DOTNET_MULTILEVEL_LOOKUP env var to zero
bool multilevel_lookup = true;

This comment was marked as spam.

This comment was marked as spam.

pal::string_t retval;
for (pal::string_t dir : hive_dir)
{
trace::verbose(_X("Searching SDK directory in [%s]"), dir.c_str());

This comment was marked as spam.

This comment was marked as spam.

Copy link
Copy Markdown
Member

@gkhanna79 gkhanna79 left a comment

Choose a reason for hiding this comment

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

Looks good modulo a refactoring comment. Please update the PR for that.


if (pal::getenv(_X("DOTNET_MULTILEVEL_LOOKUP"), &env_lookup))
{
auto env_val = pal::xtoi(env_lookup.c_str());

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@eerhardt
Copy link
Copy Markdown
Member

eerhardt commented Feb 22, 2017

Unix: the directory of “dotnet” defined in the system path.

I consider this policy to be wrong. Just because a command is in my path, doesn't mean that it should affect other commands. %ProgramFiles% on Windows != $PATH on Unix.

Take for example two machines: one that does and one that doesn't have dotnet in $PATH. Everything else being equal, my app could execute just fine on the machine with some other dotnet on the $PATH. I then publish my app, and move it to another machine, all of a sudden the app breaks because there isn't a dotnet on the $PATH.

@gkhanna79
Copy link
Copy Markdown
Member

all of a sudden the app breaks because there isn't a dotnet on the $PATH.

I do not think the app should break if there is no dotnet on path - it should only result in us determining that there is no global location for dotnet installation.

This change is implemented at
https://github.com/dotnet/core-setup/blob/master/src/corehost/common/pal.unix.cpp#L237 and was so since day 1 because there is no equivalent of PF folder on Unix to determine "global" locations. This is simply used as hint for it.

// Exe: 9999.0.0
// Expected: 9999.0.0 from exe dir
dotnet.Exec("help")
.WorkingDirectory(_currentWorkingDir)

This comment was marked as spam.

This comment was marked as spam.

@cakine
Copy link
Copy Markdown
Author

cakine commented Feb 22, 2017

I'll merge the changes tomorrow (Feb. 22nd) if there are no more concerns.

@gkhanna79
Copy link
Copy Markdown
Member

Ubuntu Arm release failure is unrelated. Merging the PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants