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

Fix installer download when temp. path is not set #18

Closed
wants to merge 8 commits into from

Conversation

mere-human
Copy link

Use a fallback TMP or TEMP and then use the current directory.

Fix #16

Use a fallback TMP or TEMP and then use the current directory.

Fix notepad-plus-plus#16
src/winmain.cpp Outdated Show resolved Hide resolved
@mere-human
Copy link
Author

How to check it without harming the local machine environment?
Just unset any of the variables in debugger. Project > Properties:
image

@mere-human
Copy link
Author

mere-human commented Aug 28, 2021

@donho I think this is pretty important to fix because:

  1. Absence of TEMP leads to crash which is not a good user experience.
  2. Using _wgetenv may lead to a security problem.

https://stackoverflow.com/questions/48568707/getenv-function-may-be-unsafe-really

@donho donho self-assigned this Aug 29, 2021
@donho
Copy link
Member

donho commented Sep 3, 2021

@mere-human

How to check it without harming the local machine environment?
Just unset any of the variables in debugger. Project > Properties:

It's good. But I need a real crash to validate this PR.
Removing both TMP & TEMP doesn't make GUP crash, and GUP updates Notepad++ with no problem.

@mere-human
Copy link
Author

But I need a real crash to validate this PR.

@donho ok, try this:

  • open a terminal (cmd)
  • navigate to Notepad++ directory
  • change environment variables only for current terminal as following
  • run set TMP=
  • run set TEMP=
  • now run notepad++.exe or gup.exe

@mere-human
Copy link
Author

Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Find a suitable solution for Notepad++ installed in Program Files.

src/winmain.cpp Outdated Show resolved Hide resolved
@mere-human mere-human marked this pull request as draft September 8, 2021 19:31
Use fallback directories: UserProfile, LocalAppData, ProgramData.
@mere-human mere-human marked this pull request as ready for review September 15, 2021 17:03
@mere-human
Copy link
Author

@donho Do you consider merging this before RC?

@jurekl
Copy link

jurekl commented Sep 17, 2021

@donho

It's good. But I need a real crash to validate this PR.
Removing both TMP & TEMP doesn't make GUP crash, and GUP updates Notepad++ with no problem.

I am providing a link to my specification of the error description #16 (comment)

Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Remove the unnecessary function. Don't copy the file into the root of user space and take consideration of empty string to terminate gracefully. Do it as simple as possible.

src/winmain.cpp Outdated Show resolved Hide resolved
src/winmain.cpp Outdated Show resolved Hide resolved
src/winmain.cpp Outdated Show resolved Hide resolved
@donho
Copy link
Member

donho commented Sep 20, 2021

@mere-human
I'm doing RC2, and I'm wondering if you wanna add %UserProfile%\Downloads in this PR.
If yes, could it be done asap? Otherwise we can always update GUP in the next next release, or RC3 (if any).

@mere-human
Copy link
Author

mere-human commented Sep 20, 2021

I'm doing RC2, and I'm wondering if you wanna add %UserProfile%\Downloads in this PR.
If yes, could it be done asap?

@donho
I can do it later today (in ~4 hours) since I need to do some testing.
If it's too late, then proceed with submitting this PR and I can add the Downloads folder handling in a separate PR.

@donho
Copy link
Member

donho commented Sep 20, 2021

If it's too late, then proceed with submitting this PR and I can add the Downloads folder handling in a separate PR.

Please do it in the same PR.
RC2 is already available, so let's integrate the new GUP in the 8.1.6 release.

@mere-human
Copy link
Author

Verified on Windows 10 and on Windows 7.
Here's a result of running an installer from terminal without TMP, TEMP and AppData:
image

@donho donho added the accept label Sep 21, 2021
@donho donho closed this in 6c9432b Sep 21, 2021
@donho donho removed the accept label Sep 21, 2021
@donho donho reopened this Sep 21, 2021
@donho
Copy link
Member

donho commented Sep 21, 2021

ARM64 build error:

image

Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Use Win32 API instead of COM, or remove the following part completely.

	LPWSTR path = nullptr;
	const HRESULT hr = ::SHGetKnownFolderPath(FOLDERID_Downloads, 0, nullptr, &path);
	if (SUCCEEDED(hr))
	{
		std::wstring result = path;
		CoTaskMemFree(path);
		return result;
	}

src/winmain.cpp Outdated Show resolved Hide resolved
@mere-human
Copy link
Author

ARM64 build error:

Sorry, I forgot about ARM64 build.
That's why WinGUP needs this #17
I'll fix it.

@mere-human
Copy link
Author

Shall we remove ARM mentions from the .vcxproj file? It seems it was added by mistake
https://github.com/notepad-plus-plus/wingup/blob/master/vcproj/GUP.vcxproj#L57
image

@donho
Copy link
Member

donho commented Sep 22, 2021

@mere-human

The request is quite clear:

Use Win32 API instead of COM, or remove the following part completely.

	LPWSTR path = nullptr;
	const HRESULT hr = ::SHGetKnownFolderPath(FOLDERID_Downloads, 0, nullptr, &path);
	if (SUCCEEDED(hr))
	{
		std::wstring result = path;
		CoTaskMemFree(path);
		return result;
	}

I don't want COM in WinGup project.

@mere-human
Copy link
Author

I don't want COM in WinGup project.

It's already in x64 build.
Ok, I can remove this part.
Or compile it for x64 but not for ARM64.

@donho
Copy link
Member

donho commented Sep 22, 2021

Shall we remove ARM mentions from the .vcxproj file? It seems it was added by mistake
https://github.com/notepad-plus-plus/wingup/blob/master/vcproj/GUP.vcxproj#L57

Leave it please.
I will take care of it myself if needed.

Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Don't modify project file.

vcproj/GUP.vcxproj Outdated Show resolved Hide resolved
@donho
Copy link
Member

donho commented Sep 23, 2021

Thank you @mere-human

@donho donho closed this in 3d5cb3f Sep 23, 2021
@mere-human mere-human deleted the not-tmp-16 branch September 23, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VERY difficult to diagnose an exception in GUP.exe, with an unspecified environment TEMP variable!
3 participants