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

Jump to bottom

Windows: Prefer USERPROFILE over HOMEPATH on startup as well #7033

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Windows: Prefer USERPROFILE over HOMEPATH on startup as well #7033

wants to merge 1 commit into from

Conversation

larskanis
Copy link
Contributor

This is a fix-up of commit d0f5dc9 .

Unfortunately the original intention to allow the use of ruby in a runas environment wasn't fixed by the above commit. This is because the win32 initialization sets the HOME environment variable based on a separate evaluation of USERPROFILE, HOMEDRIVE and HOMEPATH. And the HOME variable is preferred by Dir.home. I'm sorry I didn't know about this initialization, so that I missed to adjust this code part properly. This is now fixed.

Fixes https://bugs.ruby-lang.org/issues/19244

@larskanis larskanis mentioned this pull request Dec 27, 2022
Open
nobu
nobu reviewed Dec 29, 2022
View changes
win32/win32.c
}
else if (get_special_folder(CSIDL_PERSONAL, env, numberof(env))) {
f = TRUE;
else {
Copy link
Member

Choose a reason for hiding this comment

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

Why separated only this else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The former code was made so that if either HOMEDRIVE or HOMEPATH is defined alone, then it still is used without the other half. Maybe this a questionable behavior, but I wanted to keep that how it is.

Maybe the diff -w output is more descriptive.

@eregon
Copy link
Member

eregon commented Jan 5, 2023

@larskanis FYI https://github.com/ruby/spec/actions/runs/3849235325/jobs/6557993944 failed, so I commented ruby/spec@c181abc, please rebase and uncomment it in this PR if it passes now

@eregon
Copy link
Member

eregon commented Jan 5, 2023

Ah maybe that just needs a ruby_version_is "3.2" do guard.
The reason we see it this late, same for ruby/spec@7e680fa
is that we run ruby/spec in this on older Ruby versions but only on Linux, not on Windows or macOS. Probably we should do that to catch much earlier incorrect/missing version guards for specs added in ruby/ruby.

@larskanis
Copy link
Contributor Author

Yes, both specs are ruby-3.2 only for now. But the encoding fix is marked as to be backported to 3.1 and 3.0 in https://bugs.ruby-lang.org/issues/19243 . That's why I set the version guard to 3.0.

Should fixes always be enabled on the latest ruby only at first? And later on extended when they are backported?

@eregon
Copy link
Member

eregon commented Jan 6, 2023

Should fixes always be enabled on the latest ruby only at first? And later on extended when they are backported?

In ruby/spec's at least yes. Because REQUIRED still depends on the branch maintainer decision, and latest ruby/spec is used by and must work on CRuby e.g. 3.1 3.0 etc branches (https://rubyci.org/).

@larskanis
Copy link
Contributor Author

@eregon Makes sense. Thanks for clarification! I added the version guard and rebased the PR.

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

@nobu nobu

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@larskanis @eregon @nobu