Properly clean path for Windows machines which is *nix-compliant. #2109

Merged
merged 2 commits into from Mar 11, 2014

Projects

None yet

5 participants

@parkr
Member
parkr commented Mar 1, 2014

Fixes #1948. Modifies #2065.

/ is the FS root and Ruby will abstract this to C:/ or to / depending upon the system. The key is to actually strip out dat drive name.

/cc @XhmikosR

@XhmikosR
Contributor
XhmikosR commented Mar 2, 2014

@parkr: many thanks for looking into this! I will try it out by checking out the branch.

@XhmikosR
Contributor
XhmikosR commented Mar 2, 2014

Can you give me short instructions how to install from git on Windows? If I do bundle exec rake build I get errors

C:\Users\xmr\Desktop\jekyll>bundle exec rake build
mkdir -p pkg
gem build jekyll.gemspec
WARNING:  pessimistic dependency on liquid (~> 2.5.5) may be overly strict
  if liquid is semantically versioned, use:
    add_runtime_dependency 'liquid', '~> 2.5', '>= 2.5.5'
WARNING:  pessimistic dependency on pygments.rb (~> 0.5.0) may be overly strict
  if pygments.rb is semantically versioned, use:
    add_runtime_dependency 'pygments.rb', '~> 0.5', '>= 0.5.0'
WARNING:  pessimistic dependency on mercenary (~> 0.3.1) may be overly strict
  if mercenary is semantically versioned, use:
    add_runtime_dependency 'mercenary', '~> 0.3', '>= 0.3.1'
WARNING:  pessimistic dependency on toml (~> 0.1.0) may be overly strict
  if toml is semantically versioned, use:
    add_runtime_dependency 'toml', '~> 0.1', '>= 0.1.0'
WARNING:  prerelease dependency on jekyll-sass-converter (~> 1.0.0.rc1) is not recommended
WARNING:  pessimistic dependency on simplecov-gem-adapter (~> 1.0.1, development) may be overly strict
  if simplecov-gem-adapter is semantically versioned, use:
    add_development_dependency 'simplecov-gem-adapter', '~> 1.0', '>= 1.0.1'
WARNING:  pessimistic dependency on coveralls (~> 0.7.0, development) may be overly strict
  if coveralls is semantically versioned, use:
    add_development_dependency 'coveralls', '~> 0.7', '>= 0.7.0'
WARNING:  pessimistic dependency on activesupport (~> 3.2.13, development) may be overly strict
  if activesupport is semantically versioned, use:
    add_development_dependency 'activesupport', '~> 3.2', '>= 3.2.13'
WARNING:  open-ended dependency on jekyll_test_plugin (>= 0, development) is not recommended
  if jekyll_test_plugin is semantically versioned, use:
    add_development_dependency 'jekyll_test_plugin', '~> 0'
WARNING:  open-ended dependency on jekyll_test_plugin_malicious (>= 0, development) is not recommended
  if jekyll_test_plugin_malicious is semantically versioned, use:
    add_development_dependency 'jekyll_test_plugin_malicious', '~> 0'
WARNING:  See http://guides.rubygems.org/specification-reference/ for help
  Successfully built RubyGem
  Name: jekyll
  Version: 2.0.0.alpha.1
  File: jekyll-2.0.0.alpha.1.gem
mv jekyll-2.0.0.alpha.1.gem pkg
rake aborted!
Command failed with status (127): [mv jekyll-2.0.0.alpha.1.gem pkg...]
C:/Users/xmr/Desktop/jekyll/Rakefile:257:in `block in <top (required)>'
Tasks: TOP => build
(See full trace by running task with --trace)

EDIT: Nevermind, I used MSYS which works here.

@XhmikosR
Contributor
XhmikosR commented Mar 2, 2014

Unfortunately, it still seems to fail...

Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

C:\Users\xmr\Desktop\mpc-hc.org>ruby -v
ruby 1.9.3p484 (2013-11-22) [i386-mingw32]

C:\Users\xmr\Desktop\mpc-hc.org>SET PATH=%PATH%;C:\Python27;C:\Python27\Scripts

C:\Users\xmr\Desktop\mpc-hc.org>jekyll serve
Configuration file: C:/Users/xmr/Desktop/mpc-hc.org/_config.yml
            Source: ./source/
       Destination: ./_site/
      Generating... jekyll 2.0.0.alpha.1 | Error:  Invalid argument - C:/Users/xmr/Desktop/mpc-hc.org/_site/C:

@XhmikosR
Contributor
XhmikosR commented Mar 2, 2014

On a side note, I didn't have to specify the codepage so thanks for fixing that!

@parkr
Member
parkr commented Mar 2, 2014

I didn't have to specify the codepage so thanks for fixing that!

Oh, wow! Definitely not intentional so I have no idea why that wokred all of a sudden.

Instead of building the gem, try this:

> ruby -v
> ./bin/jekyll serve -c "C:/Users/xmr/Desktop/mpc-hc.org/_config.yml"
@XhmikosR
Contributor
XhmikosR commented Mar 2, 2014

OK, I will try as soon as I'm back home. But I think I will get the same error since I did manage to build and install the gem (see 2 comments before yours).

@parkr
Member
parkr commented Mar 3, 2014

It had a series of warnings so I thought it would be wise to try the cloned version just so you could poke around.

@XhmikosR
Contributor
XhmikosR commented Mar 3, 2014

@parkr: if I try the above I get:

Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

C:\Users\xmr\Desktop\jekyll>ruby -v
ruby 1.9.3p484 (2013-11-22) [i386-mingw32]

C:\Users\xmr\Desktop\jekyll>ruby ./bin/jekyll serve -c "C:/Users/xmr/Desktop/mpc
-hc.org/_config.yml"
Configuration file: C:/Users/xmr/Desktop/mpc-hc.org/_config.yml
            Source: ./source/
       Destination: ./_site/
      Generating... jekyll 2.0.0.alpha.1 | Error:  No such file or directory - C
:/Users/xmr/Desktop/jekyll/source/

C:\Users\xmr\Desktop\jekyll>
@parkr
Member
parkr commented Mar 3, 2014

Ah so you'll need to specify the source and destination...

./bin/jekyll serve -c "C:/Users/xmr/Desktop/mpc-hc.org/_config.yml" \
    -s "C:/Users/xmr/Desktop/mpc-hc.org/source" \
    -d "C:/Users/xmr/Desktop/mpc-hc.org/_site"
@XhmikosR
Contributor
XhmikosR commented Mar 3, 2014

Oops, sorry, I just copied and pasted that.

But the error is the same as before:

Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

C:\Users\xmr\Desktop\jekyll>ruby -v
ruby 1.9.3p484 (2013-11-22) [i386-mingw32]

C:\Users\xmr\Desktop\jekyll>ruby ./bin/jekyll serve -c "C:/Users/xmr/Desktop/mpc-hc.org/_config.yml" -s "C:/Users/xmr/Desktop/mpc-hc.org/source" -d "C:/Users/xmr/Desktop/mpc-hc.org/_site"
Configuration file: C:/Users/xmr/Desktop/mpc-hc.org/_config.yml
            Source: C:/Users/xmr/Desktop/mpc-hc.org/source
       Destination: C:/Users/xmr/Desktop/mpc-hc.org/_site
      Generating... jekyll 2.0.0.alpha.1 | Error:  Invalid argument - C:/Users/xmr/Desktop/mpc-hc.org/_site/C:

@parkr
Member
parkr commented Mar 3, 2014

Can you tell me what branch you are on? Did you git checkout proper-path-cleaning before starting this?

@XhmikosR
Contributor
XhmikosR commented Mar 3, 2014
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

C:\Users\xmr\Desktop\jekyll>git log HEAD -1
commit fb9a2a2b07626b77853d79008db6018a1600b910
Author: Parker Moore <parkrmoore@gmail.com>
Date:   Sat Mar 1 15:31:48 2014 -0500

    Properly clean path for Windows machines which is *nix-compliant.

C:\Users\xmr\Desktop\jekyll>git status
On branch proper-path-cleaning
Your branch is up-to-date with 'origin/proper-path-cleaning'.

nothing to commit, working directory clean

C:\Users\xmr\Desktop\jekyll>
@parkr
Member
parkr commented Mar 3, 2014

@XhmikosR Ok, so you are.

So here's the thing about this issue: we don't technically support Windows and it's more important to see the security hole associated with this method patched than to see everything working with Windows. Would you please take a look at Jekyll.sanitized_path in lib/jekyll.rb and try to fix it? I can't test it because I don't have a Windows computer I can test it on.

@XhmikosR
Contributor
XhmikosR commented Mar 4, 2014

I would have fixed it myself if my Ruby knowledge was better :/

I can give you access to my dev VM if you need to.

@mscharley
Contributor

@parkr What is the issue with supporting Windows? No dev's using it? If that is the case, I'd love to help out where needed as I have time. Been using Jekyll since 1.3 on Windows (with little issue actually) and it's my primary operating system as of right now at least. You know better than I where to look for Windows issues though.

@mscharley mscharley commented on an outdated diff Mar 9, 2014
lib/jekyll.rb
end
def self.sanitized_path(base_directory, questionable_path)
clean_path = File.expand_path(questionable_path, fs_root)
- clean_path.sub(/\A[\w]:\\\\/, '')
- File.join(base_directory, clean_path)
- end
-
- private
-
- def self.traverse_up(pathname)
- return pathname if pathname.parent.eql?(pathname)
- traverse_up(pathname.parent)
+ clean_path.gsub!(/\/\w\:\//, '/')
@mscharley
mscharley Mar 9, 2014 Contributor

This should read:

clean_path.gsub!(/\w\:\//, '/')

#expand_path returns a valid Windows path, this means no leading /.

@mscharley
Contributor

With the above change, this works perfectly on Windows for me.

@XhmikosR
Contributor
XhmikosR commented Mar 9, 2014

I'll try in a few hours when I'm home. Thanks for the patch!

@parkr parkr added this to the 2.0 milestone Mar 9, 2014
@parkr parkr added the Fix label Mar 9, 2014
@mattr- mattr- was assigned by parkr Mar 9, 2014
@parkr
Member
parkr commented Mar 9, 2014

Once I get confirmation from @XhmikosR, I will merge 💥

@XhmikosR
Contributor
XhmikosR commented Mar 9, 2014

With the latest patch I get:


C:\Users\xmr\Desktop\jekyll>ruby ./bin/jekyll serve -c "C:/Users/xmr/Desktop/mpc-hc.org/_config.yml" -s "C:/Users/xmr/Desktop/mpc-hc.org/source" -d "C:/Users/xmr/Desktop/mpc-hc.org/_site"
C:/RailsInstaller/Ruby1.9.3/lib/ruby/site_ruby/1.9.1/rubygems/core_ext/kernel_require.rb:126:in `require': cannot load such file -- jekyll/utils (LoadError)
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/site_ruby/1.9.1/rubygems/core_ext/kernel_require.rb:126:in `require'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-sass-converter-1.0.0.rc3/lib/jekyll/converters/sass.rb:2:in `<top (required)>'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/site_ruby/1.9.1/rubygems/core_ext/kernel_require.rb:73:in `require'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/site_ruby/1.9.1/rubygems/core_ext/kernel_require.rb:73:in `require'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/jekyll-sass-converter-1.0.0.rc3/lib/jekyll-sass-converter.rb:2:in `<top (required)>'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/site_ruby/1.9.1/rubygems/core_ext/kernel_require.rb:135:in `require'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/site_ruby/1.9.1/rubygems/core_ext/kernel_require.rb:135:in `rescue in require'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/site_ruby/1.9.1/rubygems/core_ext/kernel_require.rb:144:in `require'
        from C:/Users/xmr/Desktop/jekyll/lib/jekyll.rb:67:in `<top (required)>'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/site_ruby/1.9.1/rubygems/core_ext/kernel_require.rb:55:in `require'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/site_ruby/1.9.1/rubygems/core_ext/kernel_require.rb:55:in `require'
        from ./bin/jekyll:6:in `<main>'

C:\Users\xmr\Desktop\jekyll>
@mscharley
Contributor

I got this till I merged the current master in with this branch. The
combination of this branch and updating from master works.

@XhmikosR
Contributor
XhmikosR commented Mar 9, 2014

Yeah, that fixed it; it needs the master branch changes too. I confirm that it works then.

@parkr
Member
parkr commented Mar 10, 2014

@mattr-? Merge if you think it's good :)

@mscharley
Contributor

@parkr You've killed @mattr- 's extra commit with the update I suggested with this rebase.

@parkr
Member
parkr commented Mar 10, 2014

Whoops! Sorry, @mattr-. Fixed.

@XhmikosR
Contributor

:shipit:

@XhmikosR
Contributor

BTW, is it normal that jekyll --help shows duplicate entries? (serve and doctor)

C:\Users\xmr\Desktop\jekyll>ruby ./bin/jekyll --help
jekyll 2.0.0.alpha.1 -- Jekyll is a blog-aware, static site generator in Ruby

Usage:

  jekyll <subcommand> [options]

Options:
        -s, --source [DIR]  Source directory (defaults to ./)
        -d, --destination [DIR]  Destination directory (defaults to ./_site)
            --safe         Safe mode (defaults to false)
        -p, --plugins PLUGINS_DIR1[,PLUGINS_DIR2[,...]]  Plugins directory (defaults to ./_plugins)
            --layouts DIR  Layouts directory (defaults to ./_layouts)
        -h, --help         Show this message
        -v, --version      Print the name and version
        -t, --trace        Show the full backtrace when an error occurs

Subcommands:
  new                   Creates a new Jekyll site scaffold in PATH
  build                 Build your site
  serve                 Serve your site locally
  serve                 Serve your site locally
  doctor                Search site and print specific deprecation warnings
  doctor                Search site and print specific deprecation warnings
  docs                  Launch local server with docs for Jekyll v2.0.0.alpha.1
  import                Import your old blog to Jekyll

Also, when I do /bin/jekyll docs I get

jekyll 2.0.0.alpha.1 | Error:  undefined method `normalize_options' for main:Object
@parkr
Member
parkr commented Mar 11, 2014

is it normal that jekyll --help shows duplicate entries?

It's because of the aliases. Noted in jekyll/mercenary#25

Also, when I do /bin/jekyll docs I get underfined method normalize_options

Looks like you submitted a PR to fix this. It used to convert --drafts to a show_drafts key but I think we removed it or moved it to Configuration.

@XhmikosR
Contributor

@parkr: shouldn't doing ruby bin/jekyll show the help? Using the latest master I don't see anything om screen, and if I do ctrl+c it's like Jekyll goes into an infinite loop.

Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

C:\Users\xmr\Desktop\jekyll>ruby -v
ruby 1.9.3p484 (2013-11-22) [i386-mingw32]

C:\Users\xmr\Desktop\jekyll>ruby ./bin/jekyll
C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/option.rb:17:in `const_get': Interrupt
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/option.rb:17:in `initialize'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/command.rb:91:in `new'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/command.rb:91:in `option'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/command.rb:191:in `add_default_options'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/command.rb:156:in `go'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/program.rb:27:in `block in go'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/1.9.1/optparse.rb:882:in `initialize'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/program.rb:26:in `new'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/program.rb:26:in `go'
        from ./bin/jekyll:35:in `block (2 levels) in <main>'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/command.rb:217:in `call'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/command.rb:217:in `block in execute'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/command.rb:217:in `each'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/command.rb:217:in `execute'
        from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/mercenary-0.3.1/lib/mercenary/program.rb:35:in `go'
....
@parkr
Member
parkr commented Mar 11, 2014

Yep, that's a separate problem. Can you please file a separate issue?

@parkr parkr merged commit 215e91c into master Mar 11, 2014

1 check passed

default The Travis CI build passed
Details
@parkr parkr deleted the proper-path-cleaning branch Mar 11, 2014
@parkr parkr added a commit that referenced this pull request Mar 11, 2014
@parkr parkr Update history to reflect merge of #2109 [ci skip] 84a911f
@XhmikosR
Contributor

Done in #2124.

Are there any plans to cut a new stable release since this is merged?

@parkr
Member
parkr commented Mar 11, 2014

Thanks!

Not at the moment – we'd rather focus on getting 2.0.0 out the door :)

@XhmikosR
Contributor

I totally understand that, but I really believe there should be a bugfix release, even if it contains just this fix. :/

Like I said in a previous comment, setting up Ruby and Jekyll on Windows is already hard; having a gem release which does not work at all on Windows for so long, certainly does not help, at all.

Would it be so hard to merge this fix in the "v1-stable" branch and make a new release?

@parkr
Member
parkr commented Mar 11, 2014

I don't think so. In order to ensure that the patch release has the greatest chance of actually being fixed, would you mind taking charge of that fix? I can't test because I don't have access to a Windows machine with Ruby on it. Check out the v1-stable branch and see if the patches apply! Look for File.expand_path(\w+, ["']/["'])

@XhmikosR
Contributor

Can you tell me exactly which patches I need to merge from master? Then I will try it and make sure everything works on Windows. :)

BTW, I tried cherry picking 19dc855 and 37c2da5 but they don't apply to the "v1-stable" branch cleanly so another patch from master must be missing.

@parkr
Member
parkr commented Mar 12, 2014

master and v1-stable are very different so you'd have to go through manually and look at each occurrence of File.expand_path to determine if it's trying to clean things out. Usually in tandem with a File.join(@source, File.expand_path ...)) but not necessarily.

This is why I didn't want to do it for v1 before 2.0 is out 😜

@XhmikosR
Contributor

Maybe @perlauge can do it? Otherwise I'll have another try in the next days :)

@lmullen lmullen added a commit to lmullen/jekyll that referenced this pull request Mar 24, 2014
@parkr @lmullen parkr + lmullen Update history to reflect merge of #2109 [ci skip] dbdce22
@jekyllbot jekyllbot locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.