Windows PATH separator wrong in Chef exec command

[carpnick]

From Chef-dk 3.0 windows 7 box

From comment here:#165 (comment)

Comment:
Correct, this line needs to be using the ruby File::PATH_SEPARATOR constant: https://github.com/opscode/chef-dk/blob/7c4f55cd1665727203aef211bb3e5931d2ac8547/lib/chef-dk/helpers.rb#L86

Also it looks like we are mangling all the ";" within the path as well, sample output (Notice all ":") :

PATH=/c/opscode/chefdk/bin:C:/Users/ncarpenter/.chefdk/gem/ruby/2.0.0/bin:C:/opscode/chefdk/embedded/bin::/c/python278/:/c/ProgramData/Oracle/Java/jav
apath:/c/oracle/product/11.2.0/client_32/bin:/c/oracle/product/11.2.0/client_64/bin:/c/Program Files (x86)/Intel/iCLS Client/:/c/Program Files/Intel/i
CLS Client/:/c/Windows/system32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0/:/c/Program Files (x86)/Intel/OpenCL SD
K/3.0/bin/x86:/c/Program Files (x86)/Intel/OpenCL SDK/3.0/bin/x64:/c/Program Files/Intel/Intel(R) Management Engine Components/DAL:/c/Program Files/In
tel/Intel(R) Management Engine Components/IPT:/c/Program Files (x86)/Intel/Intel(R) Management Engine Components/DAL:/c/Program Files (x86)/Intel/Inte
l(R) Management Engine Components/IPT:/c/Program Files (x86)/QuickTime/QTSystem/:/c/Program Files/Intel/WiFi/bin/:/c/Program Files/Common Files/Intel/
WirelessCommon/:/c/Program Files (x86)/ManageSoft/Common:/c/Program Files/Microsoft SQL Server/110/Tools/Binn/:/c/Program Files (x86)/Windows Kits/8.1
/Windows Performance Toolkit/:/usr/cmd:/usr/bin:/c/Program Files (x86)/Microsoft SQL Server/100/Tools/Binn/VSShell/Common7/IDE/:/c/Program Files (x86)
/Microsoft SQL Server/100/Tools/Binn/:/c/Program Files/Microsoft SQL Server/100/Tools/Binn/:/c/Program Files (x86)/Microsoft Visual Studio 9.0/Common7
/IDE/PrivateAssemblies/:/c/Program Files (x86)/Microsoft SQL Server/100/DTS/Binn/:/c/Program Files/Microsoft/Web Platform Installer/:/c/Program Files
(x86)/Microsoft ASP.NET/ASP.NET Web Pages/v1.0/:/c/Program Files/TortoiseSVN/bin:/c/Program Files (x86)/nodejs/:/c/opscode/chefdk/bin

[lamont-granquist]

Yeah there were two different issues in one ticket, serdar closed it after appbundle'ing foodcritic, but the path fix got dropped

[sersut]

👍 Would be great if someone can get a PR for this 😄

[lamont-granquist]

I think this is the code change, it just needs specs:

e984da3

[bwmeier]

That's correct - the change works on my box.

[lamont-granquist]

^ that needs a spec and a changelog update, i've got higher priority things i'm working on for several days

[pburkholder]

@irvingpop Does this address the known issues you called out in your blog post?

[bwmeier]

Question - should GEM_PATH use ; or : as the separator on windows? It actually seems to work both ways, but the : separated path could cause problems if something in windows tries to use it natively. In my patch to chefdk on my local box, I used File::PATH_SEPARATOR in both PATH variables, along with my #187 patch.

[irvingpop]

I think it should be semicolon in PowerShell, colon in Bash (on Windows)

[lamont-granquist]

GEM_PATH should only be consumed by ruby

[irvingpop]

Confirmed:

powershell:

PS C:\Users\Irving Popovetsky> chef exec pry
c:/opscode/chefdk/embedded/lib/ruby/site_ruby/2.0.0/rubygems/dependency.rb:313:in `to_specs': Could not find 'pry' (>= 0) among 26 total gem(s) (Gem::LoadError)

Checked in 'GEM_PATH=C:/Users/Irving Popovetsky/.chefdk/gem/ruby/2.0.0:C:/opscode/chefdk/embedded/lib/ruby/gems/2.0.0;C:/Users/Irving Popovetsky/.chefdk/gem/ruby/2.0.0', execute `gem env` for more information
        from c:/opscode/chefdk/embedded/lib/ruby/site_ruby/2.0.0/rubygems/dependency.rb:322:in `to_spec'
        from c:/opscode/chefdk/embedded/lib/ruby/site_ruby/2.0.0/rubygems/core_ext/kernel_gem.rb:58:in `gem'
        from c:/opscode/chefdk/embedded/bin/pry:22:in `<main>'

bash:

$ chef exec pry
c:/opscode/chefdk/embedded/lib/ruby/site_ruby/2.0.0/rubygems/dependency.rb:313:in `to_specs': Could not find 'pry' (>= 0
) among 26 total gem(s) (Gem::LoadError)
Checked in 'GEM_PATH=c:/Users/Irving Popovetsky/.chefdk/gem/ruby/2.0.0:c:/opscode/chefdk/embedded/lib/ruby/gems/2.0.0;c:
/Users/Irving Popovetsky/.chefdk/gem/ruby/2.0.0', execute `gem env` for more information
        from c:/opscode/chefdk/embedded/lib/ruby/site_ruby/2.0.0/rubygems/dependency.rb:322:in `to_spec'
        from c:/opscode/chefdk/embedded/lib/ruby/site_ruby/2.0.0/rubygems/core_ext/kernel_gem.rb:58:in `gem'
        from c:/opscode/chefdk/embedded/bin/pry:22:in `<main>'

[sersut]

I think the scenario we want to ensure is:

# Installing a gem with a binary should be executable under chef exec context
chef gem install kitty
chef exec kitty

# A binary under embedded/bin should be able to run via chef exec
chef exec pry

[bwmeier]

FYI - I get the following errors running the rspec for 0.3.0 on Windows Powershell. These make it hard to resolve if I'm seeing different errors with this fix. I do have a couple of fixes for some of these that I can submit, but most of them would require more in-depth analysis than I have time for at the moment.

(Edit: see #225)

rspec ./spec/unit/cli_spec.rb:170 # ChefDK::CLI sanity_check! on unix complains if embedded is first
rspec ./spec/unit/cli_spec.rb:180 # ChefDK::CLI sanity_check! on unix complains if only embedded is present
rspec ./spec/unit/cli_spec.rb:190 # ChefDK::CLI sanity_check! on unix passes when both are present in the correct order
rspec ./spec/unit/cli_spec.rb:198 # ChefDK::CLI sanity_check! on unix passes when only the omnibus bin dir is present
rspec ./spec/unit/cli_spec.rb:213 # ChefDK::CLI sanity_check! on windows complains if embedded is first
rspec ./spec/unit/cli_spec.rb:223 # ChefDK::CLI sanity_check! on windows complains if only embedded is present
rspec ./spec/unit/cli_spec.rb:233 # ChefDK::CLI sanity_check! on windows passes when both are present in the correct order
rspec ./spec/unit/cli_spec.rb:241 # ChefDK::CLI sanity_check! on windows passes when only the omnibus bin dir is present
rspec ./spec/unit/cookbook_profiler/git_spec.rb:99 # ChefDK::CookbookProfiler::Git with a remote configured given a clean repo with unpublished changes reports that the repo is clean
rspec ./spec/unit/cookbook_profiler/git_spec.rb:103 # ChefDK::CookbookProfiler::Git with a remote configured given a clean repo with unpublished changes reports that there are unpublished changes
rspec ./spec/unit/cookbook_profiler/git_spec.rb:107 # ChefDK::CookbookProfiler::Git with a remote configured given a clean repo with unpublished changes reports that no remotes have the commits
rspec ./spec/unit/cookbook_profiler/identifiers_spec.rb:59 # ChefDK::CookbookProfiler::Identifiers lists the cookbook's files
rspec ./spec/unit/cookbook_profiler/identifiers_spec.rb:66 # ChefDK::CookbookProfiler::Identifiers generates a sorted list of the cookbook's files with checksums
rspec ./spec/unit/cookbook_profiler/identifiers_spec.rb:74 # ChefDK::CookbookProfiler::Identifiers generates a Hash of the cookbook's content
rspec ./spec/unit/cookbook_profiler/identifiers_spec.rb:78 # ChefDK::CookbookProfiler::Identifiers generates a dotted decimal representation of the content hash
rspec ./spec/unit/policyfile/storage_config_spec.rb:81 # ChefDK::Policyfile::StorageConfig updating storage config for policyfile location gives the expanded path to the policyfile
rspec ./spec/unit/policyfile/storage_config_spec.rb:139 # ChefDK::Policyfile::StorageConfig updating storage config for policyfile lock location gives the expanded path to the policyfile lock
rspec ./spec/unit/policyfile_lock_build_spec.rb:174 # ChefDK::PolicyfileLock building a lockfile with a minimal policyfile computes a minimal policyfile
rspec ./spec/unit/policyfile_lock_build_spec.rb:233 # ChefDK::PolicyfileLock building a lockfile with a policyfile containing a local cookbook computes a lockfile including git data
rspec ./spec/unit/policyfile_lock_build_spec.rb:427 # ChefDK::PolicyfileLock building a lockfile with a policyfile lock with a mix of cached and local cookbooks generates a lockfile with the relevant profile data for each cookbook
rspec ./spec/unit/policyfile_lock_build_spec.rb:490 # ChefDK::PolicyfileLock building a lockfile with solution dependencies specified computes a minimal policyfile
rspec ./spec/unit/policyfile_lock_build_spec.rb:598 # ChefDK::PolicyfileLock building a lockfile building a policyfile lock from a policyfile compiler generates a lockfile data structure
rspec ./spec/unit/policyfile_lock_install_spec.rb:81 # ChefDK::PolicyfileLock installing cookbooks from a lockfile Populating a PolicyfileLock from a lockfile data structure imports cached cookbook lock data
rspec ./spec/unit/policyfile_lock_install_spec.rb:94 # ChefDK::PolicyfileLock installing cookbooks from a lockfile Populating a PolicyfileLock from a lockfile data structure imports local cookbook lock data
rspec ./spec/unit/policyfile_lock_validation_spec.rb:198 # ChefDK::PolicyfileLock validating locked cookbooks when a :path sourced cookbook has changed when the cookbook has an updated version that violates no dependency constraints updates the content identifier
rspec ./spec/unit/policyfile_lock_validation_spec.rb:266 # ChefDK::PolicyfileLock validating locked cookbooks when a :path sourced cookbook has changed when a :path sourced cookbook has updated content updates the lockfile with the new checksum and validation succeeds
rspec ./spec/unit/policyfile_lock_validation_spec.rb:304 # ChefDK::PolicyfileLock validating locked cookbooks when a :path sourced cookbook has changed when a :path source cookbook has added a dependency satisfied by the current cookbook set updates the lockfile with the new checksum and validation succeeds

[btm]

Pull #232 fixed this. I'm also still seeing some remaining test failures, even after #225, but I'd like to look at those on a new issue/pr as they're unrelated to this fix.