Skip to content

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

}
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.

@larskanis
Copy link
Contributor Author

Can this be merged? Since it affects the RubyInstaller tests, all ruby release versions are already patched that way.

matzbot pushed a commit that referenced this pull request Oct 26, 2023
Enable the test commented out in d0f5dc9eac78ecade459.
Extracted from GH-7033, that is for initialization at start up time
and this test is unrelated to it.
@nobu
Copy link
Member

nobu commented Oct 26, 2023

This test in spec/ruby/core/dir/home_spec.rb seems irrelevant to the change to win32/win32.c, but the fix for d0f5dc9.

@larskanis
Copy link
Contributor Author

larskanis commented Oct 26, 2023

This test in spec/ruby/core/dir/home_spec.rb seems irrelevant to the change to win32/win32.c, but the fix for d0f5dc9.

Yes, you're right! There is no test for the startup behavior, so far. I didn't add it, since it would involve several external ruby process runs, which would slow down the specs. Still the order of the variables should be changed on startup as well. I rebased the PR to master branch. Can I do something more to get this fixed?

@nobu nobu merged commit 9a618b9 into ruby:master Oct 27, 2023
@larskanis larskanis deleted the dirhome-order2 branch October 27, 2023 07:02
larskanis added a commit to oneclick/rubyinstaller2-packages that referenced this pull request Oct 27, 2023
Because ruby/ruby#7033 was merged to ruby master branch.
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.

3 participants