Fixes for repository detection on Windows #136

GICodeWarrior · 2018-10-21T17:11:02Z

The repository detection code wasn't portable to Windows.

These changes fix detection on Windows and maintain portability with other platforms.

The existing code does not find `git.exe` on Windows, and the metadata lookup fails. This change uses PATHEXT to get the list of valid extensions and checks for each. ``` irb(main):035:0> ENV['PATHEXT'] => ".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC;.RB;.RBW" ``` This change also checks if the file is executable to avoid false positives.

Backticks don't work properly without special quoting. Switch to Open3 for portability.

DirtyF · 2018-10-21T18:02:19Z

/cc @jekyll/windows

Some dependencies don't work on 2.2. ``` Gem::RuntimeRequirementNotMetError: jekyll-watch requires Ruby version >= 2.3.0. The current ruby version is 2.2.0. An error occurred while installing jekyll-watch (2.1.2), and Bundler cannot continue. ```

GICodeWarrior · 2018-10-21T18:23:13Z

After a few tweaks, the remaining Travis failures are due to code style issues in files I've not modified.

ashmaroli · 2018-10-22T13:30:23Z

The repository detection code wasn't portable to Windows.

@GICodeWarrior Can you elaborate on how exactly you came across this issue..?
I'd like to reproduce it at my end before reviewing this proposal..

GICodeWarrior · 2018-10-22T14:24:33Z

@ashmaroli Yeah, it's the same issue described here: http://blog.johannesmp.com/2017/02/13/fixing-jekyll-serve-on-windows/#fix-no-repo-name-found

There is a "No repo name found" error on Windows even when git has a properly configured remote. Most people workaround the issue by specifying the repo name in the config. When I ran bits of the repository finder code in irb, I found that it was unable to locate git on Windows. Digging deeper, the problem was that git is git.exe on Windows, and the code did not accommodate this.

This patch fixes that problem and a follow-on issue where paths (with spaces?) on Windows don't work as expected in back-ticks style execution.

benbalter · 2018-10-22T14:35:16Z

lib/jekyll-github-metadata/repository_finder.rb

@@ -46,15 +48,19 @@ def nwo_from_config
      end

      def git_exe_path
+        exts = (ENV["PATHEXT"] || "").split(File::PATH_SEPARATOR).push("")


What's going on here?

PATHEXT on Windows is a list of file extensions respected by cmd.exe.
http://environmentvariables.org/PathExt

This code splits PATHEXT into an array of extensions and adds a blank extension to the end (for odd Windows setups and normal non-Windows setups).

irb(main):001:0> (ENV["PATHEXT"] || "").split(File::PATH_SEPARATOR).push("") => [".COM", ".EXE", ".BAT", ".CMD", ".VBS", ".VBE", ".JS", ".JSE", ".WSF", ".WSH", ".MSC", ".RB", ".RBW", ""]

On non-Windows platforms exts is only [""].

ashmaroli · 2018-10-22T15:51:15Z

lib/jekyll-github-metadata/repository_finder.rb

@@ -46,15 +48,19 @@ def nwo_from_config
      end

      def git_exe_path
+        exts = (ENV["PATHEXT"] || "").split(File::PATH_SEPARATOR).push("")
+        cmds = exts.map { |ext| "git#{ext}" }


Is there any known scenario where git.exe is not the Git Executable on Windows? If not, lets not waste time searching for those paths and zero in on ~/git.exe straight away.

I don't really have visibility into how people have their machines setup, but having a git.bat file to wrap Git is one edge case I can think of.

Random example: https://gist.github.com/duebbert/c4f26c9d0691b7d4a0e932e7d9f36271

Technically all of the extensions in PATH_EXT can be used plainly as git from the command line on Windows if someone added a wrapper or alternate implementation to their PATH.

If you want to simplify it, I recommend skipping the PATH search altogether and allowing the Open3/Exec.run call to outsource that to the OS.

Do you know why this PATH code exists in the first place?

Delegating to Windows can be bothersome IMO.
Some machines respond to where while some responds to where.exe while the others do not have either of the two programs in their PATH..

ashmaroli · 2018-10-22T15:56:34Z

lib/jekyll-github-metadata/repository_finder.rb

-          .find { |path| File.exist?(path) }
+          .map { |path| cmds.map { |cmd| File.join(path, cmd) } }
+          .flatten
+          .find { |path| File.executable?(path) && !File.directory?(path) }


Is this branching really necessary?
When is an executable ever a directory?

Directories are always executable. So if there is a git directory in PATH, we don't want to select it.

Windows:

irb(main):006:0> File.directory?('/Users') => true irb(main):007:0> File.executable?('/Users') => true

Linux:

irb(main):002:0> File.directory?('/home') => true irb(main):003:0> File.executable?('/home') => true

Today I Learned ^TM 👍

ashmaroli · 2018-10-22T15:57:26Z

@GICodeWarrior Okay, now that I have the proper context to this proposal, I understand the motivation behind this PR.
~~But my opinion on the changes proposed is that it all appears to be too hacky!~~

~~I'd personally recommend abstracting the entire "Windows support logic" into a "separate set" (or class) of methods.~~

ashmaroli

I'd like the following changes to be made:

Attribution to an existing source of information.
I found the proposed changes here to be "very similar" to an answer at StackOverflow:
It is good etiquette to attribute in FOSS, even if the original code has been "adapted"

Reduce Array traversals:

Push "" only into an empty array
Join only .exe to path strings since we're looking for only ~/git.exe
Check only if File.executable?(path) is true

Instead of chaining map, flatten, find, simply return from a nested each:

# By now, `ext` is simply [".exe"] or [""]
ENV["PATH"].to_s
  .split(File::PATH_SEPARATOR)
  .each do |path|
    exts.each do |ext|
      exe = File.join(path, "git#{ext}")
      return exe if File.executable?(exe)
    end
  end
nil

ashmaroli · 2018-10-23T13:10:51Z

lib/jekyll-github-metadata/repository_finder.rb

      end

      def git_remotes
        return [] if git_exe_path.nil?
-        `#{git_exe_path} remote --verbose`.to_s.strip.split("\n")
+        output, _status = Open3.capture2(git_exe_path, "remote", "--verbose")


@parkr @benbalter Can we drop support for Jekyll versions < 3.4.0 and use Jekyll::Utils::Exec.run instead of using Open3 directly..?

Good idea, as GitHub pages uses currently 3.7.4 it shouldn't be a problem.

GICodeWarrior · 2018-10-24T14:41:40Z

If I get back to this, I'll update with a version that removes all the PATHing code. We can defer to the OS for the correct logic and simply rescue the exception when Git is not available.

ashmaroli · 2018-10-24T14:58:37Z

@GICodeWarrior Delegating to the OS is probably not a good solution here.. (can not be reliably cross-platform)..
Please proceed by incorporating PATHEXT with the rest of the suggestions I've listed above..

GICodeWarrior · 2018-10-27T15:46:23Z

Can you clarify how allowing the platform to leverage it's own PATH mechanisms will be less reliable than a bespoke implementation of those mechanisms in this library?

GICodeWarrior · 2018-10-27T15:55:00Z

Here are some examples of how Jekyll uses Jekyll::Utils::Exec.run:

https://github.com/jekyll/jekyll/blob/0728ccf08b08759d75b6bac40091151a80556565/features/support/helpers.rb#L88-L119

ashmaroli · 2018-10-27T16:55:12Z

Can you clarify how allowing the platform to leverage it's own PATH mechanisms will be less reliable

How would a developer on UNIX-based systems go about this..?
Their system would probably have the which program installed..

$ which git
# => /path/to/git

And how would a developer on Windows go about it?
They may or may not have a which.exe program installed.
Though, there's a chance that their system probably has where.exe program installed
Even then, the invocation is different based on the shell used:

# on default Windows Command Prompt

where git
# => C:\path\to\git.exe

# on Windows PowerShell

where.exe git
# => C:\path\to\git.exe

ashmaroli · 2018-10-27T17:04:56Z

Well, it's a different story entirely if you're no longer intending to find the path to git executable at all..

ashmaroli

The re-visiting looks good to me. Just some minor comments

lib/jekyll-github-metadata/repository_finder.rb

ashmaroli · 2018-10-27T17:35:55Z

@GICodeWarrior One other thing.. We need to drop support for Jekyll versions older than v3.4.0 since the utility method doesn't exist in those versions

Co-Authored-By: GICodeWarrior <[email protected]>

ashmaroli · 2018-10-27T18:29:33Z

@parkr @benbalter Should this warrant a bump in the major version since we're dropping support for Jekyll versions older than v3.4.0..?

lib/jekyll-github-metadata/repository_finder.rb

parkr · 2018-11-05T16:54:46Z

spec/repository_finder_spec.rb

-      after(:each)  { ENV["PATH"] = @old_path.join(File::PATH_SEPARATOR) }
+      before(:each) do
+        @old_path = ENV["PATH"]
+        ENV["PATH"] = ""


The PATH is "reset" to an empty string to simulate the scenario where git is not installed.. (especially in CI environments)

parkr · 2018-11-05T16:55:27Z

spec/repository_finder_spec.rb


      it "fails with a nice error message" do
        allow(subject).to receive(:git_remote_url).and_call_original
-        expect(subject.send(:git_exe_path)).to eql(nil)


I think we still need to be able to run git? If we're relying on Jekyll::Utils::Exec, then we'll definitely need to make sure that the exe can be found, right?

Relying on Jekyll::Utils::Exec is like running system "git" in a begin..rescue block..
If the utility method doesn't find the executable, an Errno::ENOENT is raised which is rescued to output a warning to the terminal..

Ok, so we can remove git_exe_path?

Yep. In fact, this PR has already removed it..

GICodeWarrior · 2018-11-07T03:25:48Z

Updated per comments.

ashmaroli

👍 LGTM

parkr · 2018-11-07T14:54:12Z

Thank you!

@jekyllbot: merge +bug

GICodeWarrior added 2 commits October 21, 2018 09:18

Fix git execution on Windows

72b977a

Backticks don't work properly without special quoting. Switch to Open3 for portability.

DirtyF requested a review from ashmaroli October 21, 2018 18:02

DirtyF added the windows label Oct 21, 2018

GICodeWarrior added 3 commits October 21, 2018 11:03

Drop support for 2.2

d30a531

Some dependencies don't work on 2.2. ``` Gem::RuntimeRequirementNotMetError: jekyll-watch requires Ruby version >= 2.3.0. The current ruby version is 2.2.0. An error occurred while installing jekyll-watch (2.1.2), and Bundler cannot continue. ```

Include blank extension for non-Windows systems

e744b52

Fixup style

5310d03

benbalter reviewed Oct 22, 2018

View reviewed changes

ashmaroli reviewed Oct 22, 2018

View reviewed changes

DirtyF added the fix label Oct 22, 2018

ashmaroli force-pushed the master branch 2 times, most recently from 896dbc1 to de78edc Compare October 22, 2018 18:39

Merge branch 'master' into master

1dcd78e

ashmaroli force-pushed the master branch from de78edc to 1dcd78e Compare October 23, 2018 05:59

ashmaroli requested changes Oct 23, 2018

View reviewed changes

ashmaroli reviewed Oct 23, 2018

View reviewed changes

GICodeWarrior added 3 commits October 27, 2018 09:16

Switch to Jekyll::Utils::Exec

9ceab82

Remove reference to removed method

1f99e1d

Clear PATH instead of deleting

147d9fc

Remove unused require

5341d34

ashmaroli reviewed Oct 27, 2018

View reviewed changes

lib/jekyll-github-metadata/repository_finder.rb Outdated Show resolved Hide resolved

lib/jekyll-github-metadata/repository_finder.rb Outdated Show resolved Hide resolved

ashmaroli and others added 3 commits October 27, 2018 10:55

Update lib/jekyll-github-metadata/repository_finder.rb

598bc39

Co-Authored-By: GICodeWarrior <[email protected]>

Update lib/jekyll-github-metadata/repository_finder.rb

f369e51

Co-Authored-By: GICodeWarrior <[email protected]>

Drop support for versions < 3.4

27cc2d5

ashmaroli approved these changes Oct 27, 2018

View reviewed changes

ashmaroli requested a review from a team October 27, 2018 18:29

benbalter reviewed Oct 29, 2018

View reviewed changes

lib/jekyll-github-metadata/repository_finder.rb Show resolved Hide resolved

ashmaroli mentioned this pull request Oct 31, 2018

Github repo isn't being detected #142

Closed

parkr reviewed Nov 5, 2018

View reviewed changes

DirtyF added the pending-rebase label Nov 5, 2018

GICodeWarrior added 2 commits November 6, 2018 19:04

Rescue method instead of block

939614d

Merge branch 'master' into master

66e5be6

jekyllbot removed the pending-rebase label Nov 7, 2018

Return empty array, not string

3f3379b

ashmaroli approved these changes Nov 7, 2018

View reviewed changes

jekyllbot merged commit 2f5745b into jekyll:master Nov 7, 2018

jekyllbot added the bug label Nov 7, 2018

jekyllbot added a commit that referenced this pull request Nov 7, 2018

Update history to reflect merge of #136 [ci skip]

eee27c0

jekyll locked and limited conversation to collaborators Nov 7, 2019

jekyllbot added the frozen-due-to-age label Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for repository detection on Windows #136

Fixes for repository detection on Windows #136

GICodeWarrior commented Oct 21, 2018

DirtyF commented Oct 21, 2018

GICodeWarrior commented Oct 21, 2018

ashmaroli commented Oct 22, 2018

GICodeWarrior commented Oct 22, 2018

benbalter Oct 22, 2018

GICodeWarrior Oct 22, 2018

ashmaroli Oct 22, 2018

GICodeWarrior Oct 24, 2018

ashmaroli Oct 24, 2018

ashmaroli Oct 22, 2018

GICodeWarrior Oct 24, 2018

ashmaroli Oct 27, 2018

ashmaroli commented Oct 22, 2018 •

edited

Loading

ashmaroli left a comment

ashmaroli Oct 23, 2018

DirtyF Oct 23, 2018

GICodeWarrior commented Oct 24, 2018

ashmaroli commented Oct 24, 2018

GICodeWarrior commented Oct 27, 2018

GICodeWarrior commented Oct 27, 2018

ashmaroli commented Oct 27, 2018

ashmaroli commented Oct 27, 2018

ashmaroli left a comment

ashmaroli commented Oct 27, 2018

ashmaroli commented Oct 27, 2018

parkr Nov 5, 2018

ashmaroli Nov 5, 2018

parkr Nov 5, 2018

ashmaroli Nov 5, 2018

parkr Nov 5, 2018

ashmaroli Nov 5, 2018

GICodeWarrior commented Nov 7, 2018

ashmaroli left a comment

parkr commented Nov 7, 2018

Fixes for repository detection on Windows #136

Fixes for repository detection on Windows #136

Conversation

GICodeWarrior commented Oct 21, 2018

DirtyF commented Oct 21, 2018

GICodeWarrior commented Oct 21, 2018

ashmaroli commented Oct 22, 2018

GICodeWarrior commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashmaroli commented Oct 22, 2018 • edited Loading

ashmaroli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GICodeWarrior commented Oct 24, 2018

ashmaroli commented Oct 24, 2018

GICodeWarrior commented Oct 27, 2018

GICodeWarrior commented Oct 27, 2018

ashmaroli commented Oct 27, 2018

ashmaroli commented Oct 27, 2018

ashmaroli left a comment

Choose a reason for hiding this comment

ashmaroli commented Oct 27, 2018

ashmaroli commented Oct 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GICodeWarrior commented Nov 7, 2018

ashmaroli left a comment

Choose a reason for hiding this comment

parkr commented Nov 7, 2018

ashmaroli commented Oct 22, 2018 •

edited

Loading