Added support for environment variable SQUIRREL_TEMP to Setup.exe #846

LennartAJansson · 2016-10-05T11:05:36Z

Allows to control where Setup.exe will extract and execute the package
#843

Allows to control where Setup.exe will extract and execute the package

LennartAJansson · 2016-10-05T11:33:10Z

Ready to merge @paulcbetts?

anaisbetts · 2016-10-05T15:17:27Z

src/Setup/UpdateRunner.cpp

-			return -1;
-		}
+	wchar_t *envSquirrelTemp = _wgetenv(L"SQUIRREL_TEMP");
+	if (envSquirrelTemp) {


We need to check that:

This directory exists

The user has write access to it

It's not a UNC path

Ok, I will add that too :)

LennartAJansson · 2016-10-10T12:43:45Z

Ok, I will add that too :) Think we should create folder if it doesn't exist? Or just skip using it?

LennartAJansson · 2016-10-11T10:40:18Z

@paulcbetts: Added the stuff you asked for

1. SQUIRREL_TEMP directory exists 2. The user has write access to it 3. It's not a UNC path

anaisbetts · 2016-10-11T15:26:06Z

src/Setup/UpdateRunner.cpp

+		DeleteFile(szTempFileName);
+		return true;
+	}
+	catch (...) {


What part of this operation could throw a C++ exception?

GetTempFileNameW...

I don't think that throws C++ exceptions, it's going to return an error code

This is silly. Permissions could change after this is called, meaning you still have to deal with the failure in the action that proceeds this. Just do the action and skip this check.

Calling my attempt to fix the issue with the missing environmentvariable for being silly isn't really constructive @riverar ! I'm trying to contribute since I first asked for a change and if this change isn't going to be made in due time we will simply have to throw squirrel down the drains since we simply can't use it...

@LennartAJansson Oh I didn't mean that in a negative way. Poor choice of words on my part, in a repo that I've never commented in before. ☮️

That's cool @riverar :) I haven't used C++ since the 90's and never contributed to anything on github either, working with ALM instead of coding nowadays

anaisbetts · 2016-10-11T15:28:29Z

src/Setup/UpdateRunner.cpp

-		SHGetFolderPath(NULL, CSIDL_COMMON_APPDATA, NULL, SHGFP_TYPE_CURRENT, appDataDir);
-		GetUserName(username, &unameSize);
-		DWORD lastError = GetLastError();
+	if(!envSquirrelTempIsOk) {


Where do we actually use envSquirrelTemp?

_swprintf_c(targetDir, _countof(targetDir), L"%s", envSquirrelTemp);
Line 170
And if it's not set we will run the old way

envSquirrelTempIsOk was never set to true...

LennartAJansson · 2016-10-12T14:06:55Z

I found a copy and paste bug so envSquirrelTempIsOk was never set to true, sorry for that... Fixed it and pushed

riverar · 2016-10-12T22:34:16Z

src/Setup/UpdateRunner.cpp


-		_swprintf_c(targetDir, _countof(targetDir), L"%s\\%s", appDataDir, username);
+			SHGetFolderPath(NULL, CSIDL_COMMON_APPDATA, NULL, SHGFP_TYPE_CURRENT, appDataDir);


Deprecated API usage. Use SHGetKnownFolderPath instead. (You're not targeting XP users, right?)

Didn't look at that part @riverar, that's old stuff :)

riverar · 2016-10-12T22:34:41Z

src/Setup/UpdateRunner.cpp

-		_swprintf_c(targetDir, _countof(targetDir), L"%s\\%s", appDataDir, username);
+			SHGetFolderPath(NULL, CSIDL_COMMON_APPDATA, NULL, SHGFP_TYPE_CURRENT, appDataDir);
+			GetUserName(username, &unameSize);
+			DWORD lastError = GetLastError();


Nit: lastError doesn't seem to be used here.

@riverar: That part isn't mine either. I only focused on reading envvariable SQUIRREL_TEMP, it's supported in the managed part of squirrel but was missing in Setup...

riverar · 2016-10-12T22:36:11Z

.gitignore

@@ -177,3 +177,7 @@ nunit-Squirrel.Tests.xml

 ## Pester Test Output
 tests/Test.xml
+
+## CPP db crap


Nit: Something a bit more professional would be:

# Visual C++ cache files ipch/ *.aps *.ncb *.opendb *.opensdf *.sdf *.cachefile *.VC.db *.VC.VC.opendb

Agree on that, I shoudn't have pushed this change :)

LennartAJansson · 2016-10-13T10:46:18Z

Ok, I have removed the try - catch part, @paulcbetts

anaisbetts · 2016-10-13T15:30:55Z

Cool, sorry for being picky, we'll get this in today

LennartAJansson · 2016-10-17T09:30:11Z

Awesome @paulcbetts Looks like we could do some more cleaning up in the future according to the comments from @riverar

anaisbetts · 2016-10-17T21:01:00Z

Fixed!

Added support for environment variable SQUIRREL_TEMP to Setup.exe

c3def2a

Allows to control where Setup.exe will extract and execute the package

anaisbetts requested changes Oct 5, 2016

View reviewed changes

By request from paulcbetts we need to check that:

62941f8

1. SQUIRREL_TEMP directory exists 2. The user has write access to it 3. It's not a UNC path

anaisbetts requested changes Oct 11, 2016

View reviewed changes

LennartAJansson added 2 commits October 12, 2016 16:00

Corrected a bug!

3d01339

envSquirrelTempIsOk was never set to true...

Another mistake due to copy paste

8dd3e9a

riverar reviewed Oct 12, 2016

View reviewed changes

Removed try - catch in DirectoryIsWritableW

63f215e

anaisbetts merged commit 63f215e into Squirrel:master Oct 17, 2016

Thieum mentioned this pull request May 2, 2019

Policy issues #843

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for environment variable SQUIRREL_TEMP to Setup.exe #846

Added support for environment variable SQUIRREL_TEMP to Setup.exe #846

LennartAJansson commented Oct 5, 2016

LennartAJansson commented Oct 5, 2016

anaisbetts Oct 5, 2016

LennartAJansson Oct 10, 2016

LennartAJansson commented Oct 10, 2016

LennartAJansson commented Oct 11, 2016

anaisbetts Oct 11, 2016 •

edited

Loading

LennartAJansson Oct 12, 2016

anaisbetts Oct 12, 2016 •

edited

Loading

riverar Oct 12, 2016 •

edited

Loading

LennartAJansson Oct 13, 2016

riverar Oct 13, 2016

LennartAJansson Oct 13, 2016

anaisbetts Oct 11, 2016

LennartAJansson Oct 12, 2016

LennartAJansson commented Oct 12, 2016

riverar Oct 12, 2016

LennartAJansson Oct 13, 2016

riverar Oct 12, 2016

LennartAJansson Oct 13, 2016

riverar Oct 12, 2016 •

edited

Loading

LennartAJansson Oct 13, 2016

LennartAJansson commented Oct 13, 2016

anaisbetts commented Oct 13, 2016

LennartAJansson commented Oct 17, 2016

anaisbetts commented Oct 17, 2016


		_swprintf_c(targetDir, _countof(targetDir), L"%s\\%s", appDataDir, username);
		SHGetFolderPath(NULL, CSIDL_COMMON_APPDATA, NULL, SHGFP_TYPE_CURRENT, appDataDir);

Added support for environment variable SQUIRREL_TEMP to Setup.exe #846

Added support for environment variable SQUIRREL_TEMP to Setup.exe #846

Conversation

LennartAJansson commented Oct 5, 2016

LennartAJansson commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LennartAJansson commented Oct 10, 2016

LennartAJansson commented Oct 11, 2016

anaisbetts Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anaisbetts Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

riverar Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LennartAJansson commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riverar Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LennartAJansson commented Oct 13, 2016

anaisbetts commented Oct 13, 2016

LennartAJansson commented Oct 17, 2016

anaisbetts commented Oct 17, 2016

anaisbetts Oct 11, 2016 •

edited

Loading

anaisbetts Oct 12, 2016 •

edited

Loading

riverar Oct 12, 2016 •

edited

Loading

riverar Oct 12, 2016 •

edited

Loading