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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃敡 Fix windows app log path #10911

Merged
merged 6 commits into from Oct 26, 2017

Conversation

Projects
None yet
6 participants
@codebytere
Member

codebytere commented Oct 25, 2017

#10910 informed me that%HOMEPATH% and %HOMEDIR% weren't correctly evaluating per https://github.com/electron/electron/blob/master/brightray/browser/browser_main_parts.cc#L171, so I switched to using _wgetenv to get windows env variables and using them to build the path instead.

/cc @MarshallOfSound

@codebytere codebytere requested a review from electron/reviewers as a code owner Oct 25, 2017

@codebytere codebytere changed the title from :wrench: Fix windows app log path to 馃敡 Fix windows app log path Oct 25, 2017

std::wstring log_path = L"%HOMEDRIVE%%HOMEPATH%\\AppData\\Roaming\\";
std::wstring drive = _wgetenv(L"HOMEDRIVE"));
std::wstring path = _wgetenv(L"HOMEPATH"));
std::wstring log_path = drive + "\\" + path + L"\\AppData\\Roaming\\";

This comment has been minimized.

@poiru

poiru Oct 25, 2017

Member

Maybe we could inline log_path into app_log_path?

@poiru

poiru approved these changes Oct 25, 2017

馃憤

@codebytere

This comment has been minimized.

Member

codebytere commented Oct 25, 2017

now i just need to figure out why i'm killing those builds 馃槄

@alespergl

_wgetenv returns null when the variable is not defined and you will get a crash. If you insist on using environment variables, please add checks for this situation.

A better approach however is to get a "known folder" path using SHGetKnownFolderPath - use either FOLDERID_Profile if you really want the home directory (discouraged), or FOLDERID_LocalAppData for a location where apps are meant to store their runtime data.

std::wstring log_path = L"%HOMEDRIVE%%HOMEPATH%\\AppData\\Roaming\\";
std::wstring home = std::wstring(_wgetenv(L"HOMEDRIVE"));
std::wstring drive = std::wstring(_wgetenv(L"HOMEPATH"));
std::wstring log_path = home + L"\\" + drive + L"\\AppData\\Roaming\\";
std::wstring app_log_path = log_path + app_name + L"\\logs";

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 25, 2017

Member

Hm, in order to be consistent I think we should do

base::FilePath path;
PathService::Get(brightray::DIR_APP_DATA, &path);
return path + L"\\logs";

Then we don't have to worry about environment variables and the behavior RE appdata location is consistent.

This comment has been minimized.

@alespergl

alespergl Oct 25, 2017

Contributor

If there already is an abstraction like that, then awesome, let's use it instead. 馃憤

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 25, 2017

Member

@alespergl Yeah and it uses SHGetKnownFolderPath internally 馃槃

This comment has been minimized.

@alespergl

alespergl Oct 25, 2017

Contributor

... let's just make sure any failure of PathService::Get is handled and we don't end up with \logs as the final path. 馃槂

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 25, 2017

Member

Heh, good point, Get returns a success boolean so should be safe to check that

@codebytere

oh ok that looks great, I'll change that in a hot minute

@@ -168,7 +168,9 @@ BrowserMainParts::~BrowserMainParts() {
void OverrideAppLogsPath() {
#if defined(OS_WIN)
std::wstring app_name = base::UTF8ToWide(GetApplicationName());
std::wstring log_path = L"%HOMEDRIVE%%HOMEPATH%\\AppData\\Roaming\\";
std::wstring home = std::wstring(_wgetenv(L"HOMEDRIVE"));
std::wstring drive = std::wstring(_wgetenv(L"HOMEPATH"));

This comment has been minimized.

@ckerr

ckerr Oct 25, 2017

Member

I don't have much to add that hasn't already been discussed but would add one minor point: X var = X(args) is a strange way to instantiate something.

Instead, consider these (in order of my bias):

  • AAA style: auto home = std::wstring{foo};
  • uniform initializer syntax but no auto: std::wstring home {foo};
  • pre-C++11 style: std::wstring home (foo);

This comment has been minimized.

@ckerr

ckerr Oct 25, 2017

Member

Oh, also, ignorant OS_WIN question: are we guaranteed that HOMEDRIVE and HOMEPATH will always return non-null? Otherwise passing this to a wstring ctor will crash us

This comment has been minimized.

@ckerr

ckerr Oct 25, 2017

Member

d'oh, I see Marshall's suggestion supercedes all of this. Yes, that solution is better 馃憤

codebytere added some commits Oct 26, 2017

@codebytere

This comment has been minimized.

Member

codebytere commented Oct 26, 2017

only the allowed failure is still pending, so i'm going to merge

@codebytere codebytere merged commit 52cbec2 into master Oct 26, 2017

7 of 8 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
electron-mas-x64 Build #5521 succeeded in 12 min
Details
electron-osx-x64 Build #5502 succeeded in 13 min
Details

@codebytere codebytere deleted the fix_window_log_folder branch Oct 26, 2017

@kirkouimet

This comment has been minimized.

Contributor

kirkouimet commented Oct 28, 2017

Thanks @codebytere! Appreciate you jumping in and fixing this one, these folders are getting annoying. :)

Excited to get the latest build which includes your changes.

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