(PUP-11348) Use ENV directly on Windows #9052
Merged
+123
−217
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joshcooper
force-pushed
the
remove_env_monkeypatches
branch
4 times, most recently
from
9fed43d to
bb14c80
Compare
Move tests for unicode key & value and nil values from run_mode_spec.rb to util_spec.rb. We're already testing that env variables passed to `withenv` are not set when the method returns.
The test was confined to only run on Ruby < 3.0, but the minimum required version is 3.1, so delete the test. Ruby 3+ correctly uses wide Win32 APIs to access environment variables and transform them between UTF-16LE and UTF-8 regardless of the current code page. So it is possible to get/set variables whose unicode code points don't map to the current code page and that is already tested in util_spec.rb.
Don't call deprecated environment methods in Puppet::Util
Always resetting the environment is faster the compare-then-set in cases where
the environment was changed and not.
```
$ cat bench.rb
require 'benchmark/ips'
Benchmark.ips do |x|
unchanged = ENV.to_hash
changed = ENV.to_hash.merge!("foo" => "bar")
x.report("test-set unchanged") { ENV.replace(unchanged) if ENV.to_hash != unchanged }
x.report("test-set changed") { ENV.replace(unchanged) if ENV.to_hash != changed }
x.report("set unchanged") { ENV.replace(unchanged) }
x.report("set changed") { ENV.replace(changed) }
x.compare!
end
$ ruby --version
ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-linux]
$ ruby bench.rb
Warming up --------------------------------------
test-set unchanged 4.711k i/100ms
test-set changed 2.691k i/100ms
set unchanged 5.639k i/100ms
set changed 5.480k i/100ms
Calculating -------------------------------------
test-set unchanged 46.809k (± 1.1%) i/s - 235.550k in 5.032833s
test-set changed 26.749k (± 0.6%) i/s - 134.550k in 5.030353s
set unchanged 55.451k (± 0.9%) i/s - 281.950k in 5.085048s
set changed 54.355k (± 0.8%) i/s - 274.000k in 5.041297s
Comparison:
set unchanged: 55451.4 i/s
set changed: 54354.6 i/s - 1.02x slower
test-set unchanged: 46808.7 i/s - 1.18x slower
test-set changed: 26748.7 i/s - 2.07x slower
```
joshcooper
force-pushed
the
remove_env_monkeypatches
branch
from
bb14c80 to
c4428d9
Compare
joshcooper
changed the title
Ruby 3 no longer relies on the current code page when reading/writing env vars[1]. Instead it uses wide Win32 APIs and transcodes them to UTF-8. Ruby's implementation also excludes env var that start with `=` ruby/ruby@ca76337
joshcooper
force-pushed
the
remove_env_monkeypatches
branch
from
c4428d9 to
6fcaef2
Compare
tvpartytonight
approved these changes
Apr 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ruby 3 no longer uses the current code page when getting and setting environment variables. Instead it always calls wide functions like
GetEnvironmentVariableWand transcodes the value fromUTF-16LEtoUTF-8. And it does the reverse when setting environment variables. As a result, we don't need our Windows specific code for accessing environment variables.Puppet::Util.{get_env,get_environment,clear_environment,set_env,merge_environment}ENVrelated methods directlyTested Windows (x86, x64 and Japanese) in https://jenkins-platform.delivery.puppetlabs.net/view/puppet-agent/view/ad-hoc/job/platform_puppet-agent-extra_puppet-agent-integration-suite_adhoc-ad_hoc/1103/