(maint) Module install - ensure paths use / #171
Conversation
Due to https://tickets.puppetlabs.com/browse/PUP-4884, modules may fail to install on Windows. It's also somewhat hard to detect this is occurring. Grab the full path every time for install and ensure that the paths use forward slashes only.
|
CLA signed by all contributors. |
| working_dir = File.expand_path('spec/fixtures/work-dir') | ||
| working_dir = working_dir.gsub(/\\/, '/') | ||
| target_dir = File.expand_path('spec/fixtures/modules') | ||
| target_dir = target_dir.gsub(/\\/, '/') |
There was a problem hiding this comment.
Maybe a bit of overkill. 👍
There was a problem hiding this comment.
@DavidS let's chat here. I mentioned that the gsub bits are maybe a bit of overkill and probably unnecessary. This is fixing the issue with PUP-4884 in that it doesn't switch the paths to forward slashes by switching the paths to forward slashes here. I don't have a mistrust for File.expand_path, but maybe you didn't quite catch my humor here.
There was a problem hiding this comment.
I can't make the connection between PUP-4884, where a path with backslashes is specified as an argument, and this piece of code, which used hard-coded values with slashes before. Given your mistrust against File.expand_path, the updated version seems even worse than before.
Do you have a failing test log somewhere, or is this purely pro-active defensiveness?
I don't usually provide PRs for things that are just being proactive. Yes this fixes the issue. And like I said, it's maybe a bit of overkill because the gsubs probably are not required when we use expand_path. |
|
This issue is why windows modules can't modulesync up to the latest puppetlabs_spec_helper and are pinned to a particular version. It may not be the only issue, but this is a start down the correct path. |
|
Or put another way, without this change, PSH fails on Windows nearly every time. |
| " --module_working_dir spec/fixtures/module-working-dir" \ | ||
| " --target-dir spec/fixtures/modules #{remote}" | ||
| " --module_working_dir #{working_dir}" \ | ||
| " --target-dir #{target_dir} #{remote}" |
There was a problem hiding this comment.
I should have mentioned it is not super easy to see what is going on, that is why this took longer to find. This change is going from specifying a relative path to specifying a full path (using File.expand_path). The problem with the relative path is that PMT doesn't expand the path properly and so it becomes something like C:\somewhere\backslashes/spec/fixtures/work-dir on Windows, and then it fails (which is noted in PUP-4884).
There was a problem hiding this comment.
ah! this was the piece I missed. Apologies! 🤦♂️
If you could put the "The problem with the relative path..." sentence into a comment in the code, that would go a long way towards making it clear for future-us to understand what's going on here.
This wraps up the conversation from the original PR and will hopefully help us in a year when we want to remove that again.
|
|
| @@ -292,11 +292,17 @@ def max_thread_limit | |||
| end | |||
| next if File::exists?(target) | |||
|
|
|||
| # The problem with the relative path is that PMT doesn't expand the path properly and so passing in a relative path here | |||
| # becomes something like C:\somewhere\backslashes/spec/fixtures/work-dir on Windows, and then PMT barfs itself. | |||
|
Thanks for following that up! |
Due to https://tickets.puppetlabs.com/browse/PUP-4884, modules
may fail to install on Windows. It's also somewhat hard to detect
this is occurring. Grab the full path every time for install and
ensure that the paths use forward slashes only.
cc @puppetlabs/modules