Multi-level SharedFX lookup implemented. by cakine · Pull Request #1241 · dotnet/core-setup

cakine · 2017-01-21T02:18:39Z

Framework search is done in relation to the executable directory. It would be better to be able to search for it in other directories as well. Suggested folders are the current working directory, the user location and the global .NET location. The user and global folders may vary depending on the running operational system. They 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 .NET 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 · 2017-01-21T02:18:43Z

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 · 2017-01-21T02:18:50Z

@schellap

schellap · 2017-01-21T08:13:08Z

Cc @gkhanna79 @ramarag

Sign in to view

+	std::vector<pal::string_t> hive_dir;
+
+	//Multi-level SharedFX lookup happens only if dotnet_lookup env var is set to 1
+	if (pal::getenv(_X("dotnet_lookup"), &env_lookup) && env_lookup == _X("1"))


schellap · 2017-01-21T08:27:13Z

Pls. fix spacing and the PR feedback otherwise, this looks fine to me!

schellap · 2017-01-21T08:28:12Z

@ramarag this PR has changes ported from PR #1109, pls. decide on the merge order between the two.

Sign in to view

+
+		if (pal::getcwd(&cwd))
+		{
+			hive_dir.push_back(cwd);


Sign in to view

+	std::vector<pal::string_t> hive_dir;
+
+	//Multi-level SharedFX lookup happens only if dotnet_lookup env var is set to 1
+	if (pal::getenv(_X("DOTNET_ORDERED_LOOKUP"), &env_lookup) && env_lookup == _X("1"))


Sign in to view

+		append_path(&fx_dir, fx_name.c_str());
+
+		bool do_roll_forward = false;
+		if (specified_fx_version.empty())


Sign in to view

+
+		if (!do_roll_forward)
+		{
+			trace::verbose(_X("Did not roll forward because specified version='%s', patch_roll_fwd=%d, chose [%s]"), specified_fx_version.c_str(), config.get_patch_roll_fwd(), fx_ver.c_str());


Sign in to view

+		if (!do_roll_forward)
+		{
+			trace::verbose(_X("Did not roll forward because specified version='%s', patch_roll_fwd=%d, chose [%s]"), specified_fx_version.c_str(), config.get_patch_roll_fwd(), fx_ver.c_str());
+			append_path(&fx_dir, fx_ver.c_str());


Sign in to view

    bool get_own_executable_path(string_t* recv);
    bool getenv(const char_t* name, string_t* recv);
    bool get_default_servicing_directory(string_t* recv);
+	bool get_local_dotnet_dir(string_t* recv);


Sign in to view

+	pal::string_t dir;
+	if (!pal::getenv("HOME", &dir))
+	{
+		struct passwd* pw = getpwuid(getuid());


Sign in to view

+	pal::string_t path;
+	if (!pal::getenv(_X("PATH"), &path))
+	{
+		return false;


gkhanna79 · 2017-01-23T19:07:52Z

Also, can you please add tests for the new scenarios to your PR?

Sign in to view

+	std::vector<pal::string_t> hive_dir;
+
+	//Multi-level SharedFX lookup happens only if dotnet_lookup env var is set to 1
+	if (pal::getenv(_X("DOTNET_ORDERED_LOOKUP"), &env_lookup) && env_lookup == _X("1"))


gkhanna79 · 2017-01-23T22:43:47Z

@ramarag PTAL

@cakine Can you please add scenario tests as well?

Sign in to view

+    std::vector<pal::string_t> hive_dir;
+
+    //Multi-level SharedFX lookup happens only if dotnet_lookup env var is set to 1
+    bool multilevel_lookup = pal::getenv(_X("DOTNET_MULTILEVEL_LOOKUP"), &env_lookup) && env_lookup == _X("1");


gkhanna79 · 2017-01-24T00:21:44Z

Change looks good to me. Will you be able to get the tests incorporated as well?

cakine · 2017-01-24T00:26:03Z

I'm just finishing the md file right now. I still need to take a look at how the HostActivation Tests are done, though.

Sign in to view

+                {
+                    throw new DirectoryNotFoundException();
+                }
+                Directory.Delete(sharedFxDir, true);


Sign in to view

+            // completed, the folder is supposed to be empty. We're just playing safe
+            if (Directory.Exists(multilevelDir))
+            {
+                Directory.Delete(multilevelDir, true);


cakine · 2017-02-14T23:14:05Z

@mmitche @ellismg I'm facing an issue with the Fedora23 x64 build. I was not able to reproduce the bug locally (the build succeeds in my Fedora VM). Is it possible for me to access the VM where the checks are being run?

cakine · 2017-02-16T23:53:49Z

@gkhanna79

Sign in to view

+    pal::string_t env_lookup;
+
+    // Multi-level SharedFX lookup happens only if DOTNET_MULTILEVEL_LOOKUP env var is set to 1
+    bool multilevel_lookup = pal::getenv(_X("DOTNET_MULTILEVEL_LOOKUP"), &env_lookup) && (env_lookup == _X("1"));


gkhanna79 · 2017-02-17T19:41:45Z

I am good with this change. @ramarag Do you have anything to add?

gkhanna79 · 2017-02-17T21:16:48Z

@cakine I see your newly added test failing in the log (see https://ci.dot.net/job/dotnet_core-setup/job/master/job/release_windows_nt_x64_prtest/636/consoleFull):

11:12:18 [EXEC <] [FAIL] [00:00:17.928] D:\j\workspace\release_windo---0c55dee9\artifacts\win81-x64\tests\artifacts\dotnetMultilevelSharedFxLookup\2\cwd> D:\j\workspace\release_windo---0c55dee9\artifacts\win81-x64\tests\artifacts\dotnetMultilevelSharedFxLookup\2\exe\dotnet.exe "--fx-version" "9999.0.0-dummy1" "D:\j\workspace\release_windo---0c55dee9\artifacts\win81-x64\tests\artifacts\5\SharedFxLookupPortableApp\bin\SharedFxLookupPortableApp.dll" exited with -2147450749

Can you please take a look?

Sign in to view

+                .Should()
+                .Fail()
+                .And
+                .HaveStdErrContaining("It was not possible to find any compatible framework version");


gkhanna79 · 2017-02-18T04:35:40Z

I am planning to merge #1560. Once it is merged, you should rebase against it and update your negative test to invoke the Execute method as exemplified in that PR. This will keep real test passes segregated from real failures.

gkhanna79 · 2017-02-18T14:39:57Z

@cakine I have merged the PR - please rebase against it and update your test.

cakine · 2017-02-21T19:09:28Z

@gkhanna79 Done.

dnfclas added the cla-already-signed label Jan 21, 2017

schellap reviewed Jan 21, 2017

View reviewed changes

cakine force-pushed the sharedFXLookup branch from 456b412 to 70b8bde Compare January 23, 2017 18:26