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

Fix #6498: Use Gem to discover the location of bundler. #6499

Merged
merged 1 commit into from Oct 29, 2017

Conversation

Projects
None yet
5 participants
@envygeeks
Contributor

envygeeks commented Oct 29, 2017

No description provided.

@DirtyF DirtyF requested a review from jekyll/core Oct 29, 2017

@DirtyF DirtyF added the internal label Oct 29, 2017

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Oct 29, 2017

Member

@envygeeks it looks like this needs a rebase but is good to go after that.

Never mind. I don't think that's going to be an issue. Just going to wait for Travis to tell me it's green and then I'll merge.

Member

mattr- commented Oct 29, 2017

@envygeeks it looks like this needs a rebase but is good to go after that.

Never mind. I don't think that's going to be an issue. Just going to wait for Travis to tell me it's green and then I'll merge.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Oct 29, 2017

Member

Isn't the error on Appveyor related?

Member

DirtyF commented Oct 29, 2017

Isn't the error on Appveyor related?

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Oct 29, 2017

Member

Isn't the error on Appveyor related?

Looks that way. I'm going to hold off on this then until that's fixed. I know Windows isn't officially supported but I don't see a reason to merge something that we know is going to regress that platform.

Member

mattr- commented Oct 29, 2017

Isn't the error on Appveyor related?

Looks that way. I'm going to hold off on this then until that's fixed. I know Windows isn't officially supported but I don't see a reason to merge something that we know is going to regress that platform.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Oct 29, 2017

Member

just confirmed.. I was unable to run script/default-site with this branch on my Windows machine..

Member

ashmaroli commented Oct 29, 2017

just confirmed.. I was unable to run script/default-site with this branch on my Windows machine..

@mattr- mattr- dismissed their stale review Oct 29, 2017

Code looks good but the windows build needs fixing.

@mattr- mattr- requested a review from jekyll/core Oct 29, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Oct 29, 2017

Member

@envygeeks The issue here is that Windows doesn't parse the shebang #!/usr/bin/env ruby like Unix does.. so to run a ruby executable on Windows, the program creates two helper files in the bin/ directory in the %PATH% (in this case, RUBY_VERSION/bin/bundler.bat and RUBY_VERSION/bin/bundler)

But in the context of this PR, you can achieve the same by passing "ruby" to the utility function

- process, output = Jekyll::Utils::Exec.run(exe, "install")
+ process, output = Jekyll::Utils::Exec.run("ruby", exe, "install")
Member

ashmaroli commented Oct 29, 2017

@envygeeks The issue here is that Windows doesn't parse the shebang #!/usr/bin/env ruby like Unix does.. so to run a ruby executable on Windows, the program creates two helper files in the bin/ directory in the %PATH% (in this case, RUBY_VERSION/bin/bundler.bat and RUBY_VERSION/bin/bundler)

But in the context of this PR, you can achieve the same by passing "ruby" to the utility function

- process, output = Jekyll::Utils::Exec.run(exe, "install")
+ process, output = Jekyll::Utils::Exec.run("ruby", exe, "install")
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 29, 2017

Contributor

I'll get it fixed.

Contributor

envygeeks commented Oct 29, 2017

I'll get it fixed.

@mattr-

mattr- approved these changes Oct 29, 2017

LGTM

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Oct 29, 2017

Member

@jekyllbot: merge +dev

Member

mattr- commented Oct 29, 2017

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 2ecf50f into jekyll:master Oct 29, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment