(PUP-10653) Remove Dir monkey patch by GabrielNagy · Pull Request #8314 · puppetlabs/puppet

GabrielNagy · 2020-09-03T12:16:06Z

We removed our external dependency on the win32-dir gem in #8258 by pulling the gem code in puppet. However, it seems that win32-dir features are not used outside of constants which can all be replaced by environment variables. Do that, and remove the monkey patched code.

joshcooper · 2020-09-03T15:07:27Z

Some background, we started using COMMON_APPDATA in 95b21df, because we needed to support 2003 and newer versions, and it abstracted the differences between %ALLUSERSPROFILE%\Application Data vs %SYSTEMDRIVE%\ProgramData. We dropped 2003 support awhile ago, so that abstraction is not needed.

Puppet modules/extensions/code often needs to know where the confdir, vardir, etc are, for example to find puppet.conf, facts.d, etc, so there are a fair number of repos in the puppetlabs namespace that explicitly require 'win32/dir' and access the Dir::COMMON_APPDATA constant. And almost certainly that pattern has been copied to external modules:

https://github.com/puppetlabs/puppet-enterprise-modules/blob/64f37498654d8c1f20aeb3e48875777e4bfaad3a/modules/puppet_enterprise/lib/facter/windows.rb#L5
https://github.com/puppetlabs/puppetlabs-pe_common/blob/149a87d17d5d7603b1b4ee09badad8bfa9e474e7/lib/facter/windows.rb#L5
https://github.com/puppetlabs/puppetlabs-stdlib/blob/7a6e34667266c9767cb549cbccae310e1917b3a1/lib/facter/facter_dot_d.rb#L211
https://github.com/puppetlabs/bolt/blob/e5c3c03cf2ed864182b8f53cd8defa7cd491c9e2/lib/bolt/puppetdb/config.rb#L22

So I'm a little reluctant to remove that constant entirely... Maybe remove the overall win32/dir gem monkey patch and define the COMMON_APPDATA constant based on the environment variable? However we'd also need to define an empty lib/win32/dir.rb file so the require 'win32/dir' doesn't blow up. Or we could skip that part?

A few things to watch out for though. The win32/dir monkey patches Dir.pwd to use backslashes instead of ruby's normal forward slash:

C:\Users\josh\projects\puppet>bundle exec ruby -e "puts Dir.pwd"
C:/Users/josh/projects/puppet
C:\Users\josh\projects\puppet>bundle exec ruby -e "require 'win32/dir'; puts Dir.pwd"
C:\Users\josh\projects\puppet

We had issues due to that difference (fixed in 6767686, aa67f14). We might need to check if there are any calls to Dir.{entries|glob} with a relative path to pwd?

Another place this might cause issues is SmartPath, which is used to restrict where we search for things to load, as it relies on starts_with?. So if there's a mix of forward and backward slashes we may revert to scanning "all of the things". It's also possible that code doesn't work on Windows due to case insensitive paths?

puppetcla · 2020-09-03T18:00:14Z

CLA signed by all contributors.

GabrielNagy · 2020-09-03T18:56:04Z

Since almost all the places that access the constant also require win32/dir, I'd be more inclined to update the modules to use the environment variable. Even with the monkeypatch present the require statement would still fail as we no longer vendor the gem.

Bolt as a gem will probably need to have an explicit dependency on the gem, while packaged Bolt will still work as their runtime includes the win32 gems: https://github.com/puppetlabs/puppet-runtime/blob/26a3d6eaf8f1ef750e73d785583a60c48a7495ec/configs/projects/bolt-runtime.rb#L197.

I think it's gnarlier if we keep the monkey patch and another project requires win32/dir on top of it as they'd get warnings due to redefined constants (which can be mitigated on our side, but still). It seems that either way we choose to do it, those places will need to be updated.

GabrielNagy · 2020-09-04T14:24:32Z

Ok, I went through all the places where we call glob and [] and we don't appear to call the methods with backslash-separated paths. I annotated all occurences here https://gist.github.com/GabrielNagy/bbe659d8268d924d668f91b3f05a7b76, nothing looks problematic.

Dir.pwd is used here: https://github.com/puppetlabs/puppet/blob/main/lib/puppet/application/resource.rb#L176
Calling #path on this produces a mix of forward and backslashes if win32/dir is loaded, but this is used only when running puppet resource with --edit which doesn't seem to work on Windows. So at best, it would be an improvement if we remove the monkey patched Dir.pwd.

About SmartPath, start_with? is called with generic_path which is always something that goes through File.join which uses forward slashes.

6767686 and aa67f14 don't cause any breakage after removing the monkey patched code, but I'd still revert them to avoid eventual confusion. Thoughts @joshcooper?

joshcooper · 2020-09-04T19:08:42Z

  @LOG_TO_FILE = false
  @loglevel = 0
-  LOG_FILE =  File.expand_path(File.join(Dir::COMMON_APPDATA, 'PuppetLabs', 'puppet', 'var', 'log', 'windows.log'))
+  LOG_FILE =  File.expand_path(File.join(ENV['ALLUSERSPROFILE'], 'PuppetLabs', 'puppet', 'var', 'log', 'windows.log'))


If the service is configured to run under a domain service account, would COMMON_APPDATA be different than ALLUSERSPROFILE? I assume not, but wanted to check

I set up a domain controller and client, set the puppet service to run as a domain account and confirmed that COMMON_APPDATA and ALLUSERSPROFILE are the same.

joshcooper · 2020-09-04T19:10:33Z

👍 to reverting the commits above to avoid confusion

GabrielNagy · 2020-09-07T11:24:15Z

I reverted the rdoc commit, the other one is not applicable anymore since builder.rb was removed in 8e53afb.

joshcooper

LGTM. There's a conflict that needs resolving. Also could you run with pr tests on 32 and 64 bit Windows?

Remove usage of CSIDL Dir constants and replace them with environment variables. Dir::PROFILE -> ENV['USERPROFILE'] Dir::COMMON_APPDATA -> ENV['ALLUSERSPROFILE'] Dir::WINDOWS -> ENV['SystemRoot'] Remove requires to the Dir monkey patch.

This reverts commit 6767686. As we are removing the Dir (win32/dir) monkeypatches, this workaround is no longer needed.

GabrielNagy · 2020-09-10T07:40:13Z

jenkins please test this on windows10ent-64,windows10ent-32,windows2012r2-64,windows2016-64,windows2019-64,windows2012-64,windows2012r2-64,windows2012r2_wmf5-64,windows2012r2_core-64,windows7-64,windows81-64,windows10ent-32,windows10ent-32,windows2016-64,windows2016_core-64,windows2019_core-64,windows2019_ja-64a

For Puppet 7 we are dropping the `win32/dir` dependency as we only used constants from it, which we replaced with environment variables (see: puppetlabs/puppet#8314). This would become breaking in bolt when it switches to Puppet 7 and the win32 gems are no longer pulled. For AIO bolt, the Puppet change won't break anything since bolt uses its own runtime which still packages the win32 gems, but it would still be an improvement in speed if the dependency is dropped. The `win32/dir` gem also monkey patches some `Dir` methods, most notably `glob` and `pwd` to use backslashes which end up causing more trouble as Ruby uses `/` as the default separator for Windows. Switch to using the `ALLUSERSPROFILE` environment variable instead of the `Dir::COMMON_APPDATA` constant, and remove a `Dir.pwd` workaround.

For Puppet 7 we are dropping the `win32/dir` dependency as we only used constants from it, which we replaced with environment variables (see: puppetlabs/puppet#8314). This would become breaking in bolt when it switches to Puppet 7 and the win32 gems are no longer pulled. For AIO bolt, the Puppet change won't break anything since bolt uses its own runtime which still packages the win32 gems, but it would still be an improvement in speed if the dependency is dropped. The `win32/dir` gem also monkey patches some `Dir` methods, most notably `glob` and `pwd` to use backslashes which end up causing more trouble as Ruby uses `/` as the default separator for Windows. Switch to using the `ALLUSERSPROFILE` environment variable instead of the `Dir::COMMON_APPDATA` constant, and remove a `Dir.pwd` workaround. !removal * **Remove dependency on win32/dir** ([PUP-10653](https://tickets.puppetlabs.com/browse/PUP-10653)) Replace the usage of win32/dir constants with environment variables.

For Puppet 7 we are dropping the `win32/dir` dependency as we only used constants from it, which we replaced with environment variables (see: puppetlabs/puppet#8314). This would become breaking when using the stdlib module with Puppet 7, as the win32 dependencies are no longer provided in the puppet gem. Replace the usage of the `Dir::COMMON_APPDATA` with the `ALLUSERSPROFILE` environment variable.

For Puppet 7 we are dropping the `win32/dir` dependency as we only used constants from it, which we replaced with environment variables (see: puppetlabs/puppet#8314). This would become breaking in bolt when it switches to Puppet 7 and the win32 gems are no longer pulled. For AIO bolt, the Puppet change won't break anything since bolt uses its own runtime which still packages the win32 gems, but it would still be an improvement in speed if the dependency is dropped. The `win32/dir` gem also monkey patches some `Dir` methods, most notably `glob` and `pwd` to use backslashes which end up causing more trouble as Ruby uses `/` as the default separator for Windows. Switch to using the `ALLUSERSPROFILE` environment variable instead of the `Dir::COMMON_APPDATA` constant, and remove a `Dir.pwd` workaround. !removal * **Remove dependency on win32/dir** ([PUP-10653](https://tickets.puppetlabs.com/browse/PUP-10653)) Replace the usage of win32/dir constants with environment variables.

For Puppet 7 we are dropping the `win32/dir` dependency as we only used constants from it, which we replaced with environment variables (see: puppetlabs/puppet#8314). This would become breaking when using the stdlib module with Puppet 7, as the win32 dependencies are no longer provided in the puppet gem. Replace the usage of the `Dir::COMMON_APPDATA` with the `ALLUSERSPROFILE` environment variable.

GabrielNagy requested review from a team September 3, 2020 12:16

GabrielNagy added the Night's Watch label Sep 3, 2020

GabrielNagy force-pushed the PUP-10653/remove-dir-monkeypatch branch from bc0af0f to 36f0f77 Compare September 4, 2020 07:50

joshcooper reviewed Sep 4, 2020

View reviewed changes

joshcooper reviewed Sep 9, 2020

View reviewed changes

GabrielNagy added 3 commits September 10, 2020 09:55

(PUP-10653) Remove usage of Dir constants

801c433

Remove usage of CSIDL Dir constants and replace them with environment variables. Dir::PROFILE -> ENV['USERPROFILE'] Dir::COMMON_APPDATA -> ENV['ALLUSERSPROFILE'] Dir::WINDOWS -> ENV['SystemRoot'] Remove requires to the Dir monkey patch.

(PUP-10653) Remove Dir monkey patch

28f69b2

Revert "(PUP-391) Fix Rdoc 4+ specs on Windows Ruby 2 x64"

63c6020

This reverts commit 6767686. As we are removing the Dir (win32/dir) monkeypatches, this workaround is no longer needed.

GabrielNagy force-pushed the PUP-10653/remove-dir-monkeypatch branch from 1f88d56 to 63c6020 Compare September 10, 2020 06:57

luchihoratiu approved these changes Sep 10, 2020

View reviewed changes

GabrielNagy mentioned this pull request Sep 10, 2020

(PUP-10653) Remove dependency on win32/dir puppetlabs/bolt#2161

Merged

GabrielNagy mentioned this pull request Sep 10, 2020

(PUP-10653) Remove win32/dir constant usage puppetlabs/puppetlabs-stdlib#1125

Merged

joshcooper approved these changes Sep 10, 2020

View reviewed changes

joshcooper merged commit e677808 into puppetlabs:main Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PUP-10653) Remove Dir monkey patch#8314

(PUP-10653) Remove Dir monkey patch#8314
joshcooper merged 3 commits intopuppetlabs:mainfrom
GabrielNagy:PUP-10653/remove-dir-monkeypatch

GabrielNagy commented Sep 3, 2020

Uh oh!

joshcooper commented Sep 3, 2020

Uh oh!

puppetcla commented Sep 3, 2020

Uh oh!

GabrielNagy commented Sep 3, 2020

Uh oh!

GabrielNagy commented Sep 4, 2020

Uh oh!

joshcooper Sep 4, 2020

Uh oh!

GabrielNagy Sep 7, 2020

Uh oh!

joshcooper commented Sep 4, 2020

Uh oh!

GabrielNagy commented Sep 7, 2020

Uh oh!

joshcooper left a comment

Uh oh!

GabrielNagy commented Sep 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

GabrielNagy commented Sep 3, 2020

Uh oh!

joshcooper commented Sep 3, 2020

Uh oh!

puppetcla commented Sep 3, 2020

Uh oh!

GabrielNagy commented Sep 3, 2020

Uh oh!

GabrielNagy commented Sep 4, 2020

Uh oh!

joshcooper Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

GabrielNagy Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

joshcooper commented Sep 4, 2020

Uh oh!

GabrielNagy commented Sep 7, 2020

Uh oh!

joshcooper left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielNagy commented Sep 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants