Skip to content

Conversation

@hsbt
Copy link
Member

@hsbt hsbt commented Oct 4, 2024

This changes are for syncing ruby/ruby repo.

  • ruby/ruby repo needs test-unit instead of minitest.
  • I reverted f5ea80d. Because this is for [win32/registry] Fallback to UTF-8 for unknown codepages ruby#7366. I don't have Arabic environment, but this issue is reported in 2023. Is it working only UTF8 locale? The current ruby/ruby implementation may worked at least.
  • helper file is not recommended at ruby/ruby repo. I embedded it to test file.
  • I'm not sure what's better to use volatile environment and (unused_value)[https://github.com/ruby/ruby/blob/master/test/win32/test_registry.rb#L86] block. This repository don't use that. What's do you think that?

@larskanis Can you look the above changes? After merging this, I enabled auto-sync feature this repository and ruby/ruby repository. You can develop this repository independently from ruby/ruby repo and we integrate these changes automatically for new Ruby release.

Unfortunately, we don't have sync feature for ruby/ruby to ruby/win32-registry. If we have those cases, I will create PRs manually.

@hsbt hsbt force-pushed the forward-port-from-ruby-master branch 4 times, most recently from 39cf817 to 369129c Compare October 4, 2024 05:56
@hsbt hsbt marked this pull request as ready for review October 4, 2024 08:39
@hsbt
Copy link
Member Author

hsbt commented Oct 4, 2024

This change is testing now at ruby/ruby#11791

@larskanis
Copy link
Collaborator

larskanis commented Oct 4, 2024

  • ruby/ruby repo needs test-unit instead of minitest.
  • helper file is not recommended at ruby/ruby repo. I embedded it to test file.

Both is OK, of course. Thank you!

  • I'm not sure what's better to use volatile environment and (unused_value)[https://github.com/ruby/ruby/blob/master/test/win32/test_registry.rb#L86] block. This repository don't use that. What's do you think that?

Using a random subkey versus a fixed one is probably necessary to run the tests in parallel. This is no problem in this repository, but in ruby/ruby. I can change this.

On the other hand I don't like having to clean up every test case per ensure path. This is tedious and should be done in a setup/teardown method IMHO.

  • I reverted f5ea80d. Because this is for #7366 . I don't have Arabic environment, but this issue is reported in 2023. Is it working only UTF8 locale? The current ruby/ruby implementation may worked at least.

Both ruby/ruby#7366 and oneclick/rubyinstaller2#348 are fixed by just using UTF-8 as LOCALE. #7366 is outdated insofar, that I patched RubyInstaller to use UTF-8 always, since a year ago or so. So reverting it doesn't make sense to me.

There have been even more bug reports regarding non-UTF-8 registry keys, so that I decided to change it to consistent UTF-8 output. I only forgot to send the patch to ruby/ruby. But I created the win32-registry repo because of this inconsistent use of locale/UTF-8 string encoding and to have a way to properly run tests on the code. In contrast I didn't receive a single bug report due to changing the output to consistent UTF-8.

If f5ea80d is reverted, then the "test_utf8_encoding" test fails on non UTF-8 code pages. But not on GHA, since it is switched to 65001.

@larskanis larskanis force-pushed the forward-port-from-ruby-master branch from 7c6e9a8 to f66254e Compare October 4, 2024 19:16
@larskanis larskanis merged commit b80ba00 into master Oct 4, 2024
@larskanis
Copy link
Collaborator

I removed the reverted commit and merged this PR, so that I can check the parallel test and random registry path. Please raise your hand if the UTF-8 patch is still a concern for you!

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