Add global registered install location to Linux and macOS#6124
Conversation
vitek-karas
changed the title
|
/cc @jeffschwMSFT |
The product now reads the /etc/dotnet/install_location file and use its first line as the path to the install location. Adds test-only env. variables to be able to test the behavior around picking global install location. Changes windows-only tests to run on Linux/macOS as well now. Added some tests. Improved the test infra around global registered location.
vitek-karas
force-pushed
the
InstallLocation
branch
from
0402512 to
f46d7e5
Compare
|
|
||
| // ***Used only for testing*** | ||
| pal::string_t environment_override; | ||
| if (pal::getenv(_X("_DOTNET_TEST_GLOBALLY_REGISTERED_PATH"), &environment_override)) |
There was a problem hiding this comment.
I thought you were worried about a secutiry hole with this.
There was a problem hiding this comment.
Yes - but I didn't want to have one PR solving two separate issues... and I need a test-only thing for this feature. So I kept doing it the "old" way... will fix this in a separate PR for all cases where we do this.
| [Theory] | ||
| [InlineData("{0}", true)] | ||
| [InlineData("{0}\n", true)] | ||
| [InlineData("{0}\nSome other text", true)] |
There was a problem hiding this comment.
It is not clear why we should allow this case to be supported.
There was a problem hiding this comment.
This is for extensibility. If in the future we needed to add more stuff into the file, the "old" apphosts should not fail on it. So we intentionally only read the first line and ignore the rest.
sdmaclea
left a comment
sdmaclea
left a comment
There was a problem hiding this comment.
Looks fine to me.
- Supporting multiple lines in the install location file seems suspicious.
- I thought adding an environment variable for testing had security risks.
- Using the registry and fiddling with the registry feels icky. Apparently it is required by the current design.
- I wonder if running the tests in a container with full admin rights might offer a better alternative
elinor-fung
left a comment
elinor-fung
left a comment
There was a problem hiding this comment.
Just the same question as @sdmaclea about the environment variables.
| bool get_default_servicing_directory(string_t* recv); | ||
|
|
||
| //On Linux, there are no global locations | ||
| //On Windows there will be up to 2 global locations |
There was a problem hiding this comment.
This is still true, no?
Is it worth calling out the distinction between global / registered / default and how they are expected to be used for fxr / framework and sdk / shared store paths?
There was a problem hiding this comment.
Even before this change Linux does have a global location, and now it also has the registered location.
I'm not sure we should have the full description in the header file - it's relatively complex as it stands right now...
There was a problem hiding this comment.
Oh, I read this comment as referring to get_global_dotnet_dirs, which still does not return any directories on Linux. That seemed wrong to me (in my mind global would include the default and registered location), but then I realized that it was not used for fxr lookup and only multi-level framework/sdk/store - hence my thinking the distinction was unclear.
I guess I just found that name (get_global_dotnet_dirs) confusing, since it doesn't quite match what I think of as 'global'.
There was a problem hiding this comment.
I added comments to the three functions which are confusing... The names are actually technically correct, but I understand the confusion. Maybe I'm just too "in this" to come up with better names... If you have better ideas please share...
|
|
||
| using (RegisteredInstallLocationOverride registeredInstallLocationOverride = new RegisteredInstallLocationOverride()) | ||
| { | ||
| File.WriteAllText(registeredInstallLocationOverride.PathValueOverride, string.Format(value, installLocation)); |
There was a problem hiding this comment.
registeredInstallLocationOverride.SetInstallLocation?
There was a problem hiding this comment.
That would probably work, but I intentionally used the explicit file write here because this is testing values which are not just locations. So basically this should be the only test which does not use the SetInstallLocation.
Extensibility - if one day we need to add some more information into this file, we don't want for the old apphosts to break.
Yes it is. I'm preparing a separate change to mitigate that, but I didn't want to combine it with this PR, would be too confusing. So I kept doing this the old way for now.
I agree it's not very nice. Most tests don't need that though as they use the env. variable which simply overrides what the registry returns. Unfortunately we need at least a couple tests which actually do run the registry reading code. I added this some time back when there was a bug in the registry reading code which ment it didn't work on downlevel Windows.
I actually think it would be great to have a separate suite of tests which are more end-to-end and which would need to run in a container and with admin rights. Those could actually "install" the runtime and then try to run a real app and so on. I created #6262 to track this. |
…e-setup#6124) The product now reads the /etc/dotnet/install_location file and use its first line as the path to the install location. Adds test-only env. variables to be able to test the behavior around picking global install location. Changes windows-only tests to run on Linux/macOS as well now. Added some tests. Improved the test infra around global registered location. Commit migrated from dotnet/core-setup@40aeca5
3 participants
The product now reads the /etc/dotnet/install_location file and use its first line as the path to the install location.
Implementation of design described in dotnet/designs#58
Adds test-only env. variables to be able to test the behavior around picking global install location.
Changes windows-only tests to run on Linux/macOS as well now.
Added some tests.
Improved the test infra around global registered location.