-
Notifications
You must be signed in to change notification settings - Fork 102
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
ENT-11959: Fixed modules_presence bundle for Windows agents #2923
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look dangerous. Though I don't fully understand the reasoning.
windows:: | ||
"_package_paths_tmp" slist => findfiles("\Q$(_override_dir)\E*"); | ||
"_package_paths_tmp" slist => findfiles("$(_override_dir)*"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the treatment as literal string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, because this.promise_dirname on windows contains backslashes. SO this change switches to using only backslashes on windows. but what were we protecting against with \Q \E?. findfiles() is glob, but \Q\E is pcre right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because findfiles doesn't work with those in there.
If I leave these out I get
R: _override_dir: C:\Program Files\Cfengine\inputs\cfe_internal\update\..\..\modules\packages\
R: _package_paths_tmp: [
"C:\\Program Files\\Cfengine\\inputs\\cfe_internal\\update\\..\\..\\modules\\packages\\craig.mustache",
"C:\\Program Files\\Cfengine\\inputs\\cfe_internal\\update\\..\\..\\modules\\packages\\vendored"
]
If I leave them in I get
R: _override_dir: C:\Program Files\Cfengine\inputs\cfe_internal\update\..\..\modules\packages\
R: _package_paths_tmp: []
I guess that new findfiles() handles windows paths with back-slashes in a different way which "just works" now?
Maybe because it uses true/fixed globbing where before it used PCRE?
@larsewi would know for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notice the acceptance tests for findfiles on windows...
All back-slashes
windows::
"expected[a]" string => "$(G.testdir)\\a,$(G.testdir)\\d,$(G.testdir)\\g";
"expected[b]" string => "$(G.testdir)\\a,$(G.testdir)\\bc,$(G.testdir)\\d,$(G.testdir)\\g,$(G.testdir)\\klm";
"expected[c]" string => "$(G.testdir)\\d\\e,$(G.testdir)\\g\\h";
"expected[d]" string => "$(G.testdir)\\a,$(G.testdir)\\bc";
"expected[e]" string => "";
"expected[g]" string => "$(G.testdir)\\a,$(G.testdir)\\bc,$(G.testdir)\\d,$(G.testdir)\\g,$(G.testdir)\\klm,$(G.testdir)\\d\\e,$(G.testdir)\\g\\h,$(G.testdir)\\klm\\nop,$(G.testdir)\\d\\e\\f,$(G.testdir)\\g\\h\\i,$(G.testdir)\\klm\\nop\\qrs,$(G.testdir)\\g\\h\\i\\j";
"expected[h]" string => "$(G.testdir)\\g\\h\\i\\j";
With findfiles returning proper back-slashed paths this policy was broken due to _vendored_dir and _override_dir being used with mixed slashed path and used as a replacement string. Ticket: ENT-11959 Changelog: none
aa4d04b
to
4f859f9
Compare
@@ -860,6 +861,8 @@ bundle agent modules_presence | |||
|
|||
reports: | |||
DEBUG:: | |||
"_override_dir: $(_override_dir)"; | |||
"_package_paths_tmp: $(with)" with => storejson(_package_paths_tmp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added _override_dir
and _package_paths_tmp
for debugging on windows.
no cherry pick can be made, 3.21.x operates differently with globs and windows. :) |
With findfiles returning proper back-slashed paths this policy was broken due to _vendored_dir and _override_dir being used with mixed slashed path and used as a replacement string.
Ticket: ENT-11959
Changelog: none