Execute jekyll from clone instead of defined binary when running 'script/default-site' #5295

Merged
merged 3 commits into from Aug 30, 2016

Conversation

Projects
None yet
6 participants
@ashmaroli
Member

ashmaroli commented Aug 28, 2016

While working on #5241, I noticed that its not required to run jekyll new from exe/jekyll binary and rather works well with Jekyll cloned with the build.
I opened this PR to have all discussions and updates separate from the other PR.

/cc @jekyll/ecosystem
/cc @jekyll/windows

@ashmaroli ashmaroli changed the title from Execute jekyll from clone instead of defined binary to Execute jekyll from clone instead of defined binary when running 'script/default-site' Aug 28, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
Contributor

benbalter commented Aug 29, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Aug 29, 2016

Contributor

@ashmaroli Can you describe a bit more what you're trying to do here? I'm having trouble following the diff.

Contributor

benbalter commented Aug 29, 2016

@ashmaroli Can you describe a bit more what you're trying to do here? I'm having trouble following the diff.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 29, 2016

Member

okay, I am running bundle exec jekyll new here instead of bundle exec exe/jekyll new
Why? because this script fails on windows as exe/jekyll is not identified as an executable.

Additionally, changed workdir=$(pwd) to workdir="../../"
I also removed $0 from every echo

Member

ashmaroli commented Aug 29, 2016

okay, I am running bundle exec jekyll new here instead of bundle exec exe/jekyll new
Why? because this script fails on windows as exe/jekyll is not identified as an executable.

Additionally, changed workdir=$(pwd) to workdir="../../"
I also removed $0 from every echo

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 29, 2016

Member

If its really necessary, I'll split the commit into bite-size ones.

Member

ashmaroli commented Aug 29, 2016

If its really necessary, I'll split the commit into bite-size ones.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Aug 29, 2016

Contributor

First of all, I already have #5240 which includes the necessary part of this patch.

The rest of the changes seem unneeded to me at this point.

Contributor

XhmikosR commented Aug 29, 2016

First of all, I already have #5240 which includes the necessary part of this patch.

The rest of the changes seem unneeded to me at this point.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 29, 2016

Member

@XhmikosR, I'm aware of that PR. this PR deals with discussing bundle exec jekyll new instead of bundle exec exe/jekyll, and the relevance of exe/jekyll in this script.
It has nothing to do with bundle exec ruby exe/jekyll new

Member

ashmaroli commented Aug 29, 2016

@XhmikosR, I'm aware of that PR. this PR deals with discussing bundle exec jekyll new instead of bundle exec exe/jekyll, and the relevance of exe/jekyll in this script.
It has nothing to do with bundle exec ruby exe/jekyll new

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Aug 29, 2016

Contributor

Then just make the needed changes only instead of changing the log messages for example.

Contributor

XhmikosR commented Aug 29, 2016

Then just make the needed changes only instead of changing the log messages for example.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 29, 2016

Member

Roger that, Captain! 😄

Member

ashmaroli commented Aug 29, 2016

Roger that, Captain! 😄

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 29, 2016

Member

Additionally, changed workdir=$(pwd) to workdir="../../"

Out of curiosity, why did you need to make this change? The updated code in this PR is harder to follow in my opinion so I would love to know why it is needed. Thanks!

Member

parkr commented Aug 29, 2016

Additionally, changed workdir=$(pwd) to workdir="../../"

Out of curiosity, why did you need to make this change? The updated code in this PR is harder to follow in my opinion so I would love to know why it is needed. Thanks!

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 30, 2016

Member

why did you need to make this change?

@parkr, I'll try to explain things to the best of my ability.
The main reason why you are unable to understand this is because you've personally not tested it out on a windows-machine per se. So I'll include references to Appveyor logs that you can verify.

value $(pwd) _before_ $0: creating new default site == _/c/projects/jekyll_

say, I move assigning to $(pwd) further down (after pushd temp/default-site), then
$(pwd) == _/c/projects/jekyll/tmp/default-site_ ( An incorrect path on Windows )

But.. when I assign workdir to simply be two directories up ( _../../_ ) _after_ pushd tmp/default-site, then
in-theory, its a place where one can expect to find Jekyll gemspec, on any platform.

Rightly so, Both Appveyor and Travis exits green, after executing every line.

This explained why I changed the value of workdir


Refs:
Failed test in Appveyor: https://ci.appveyor.com/project/jekyll/jekyll/build/386/job/vyjf304g5ovbkogn
Passed test in Appveyor: https://ci.appveyor.com/project/jekyll/jekyll/build/403/job/ryc5t0pkc9egovox

Member

ashmaroli commented Aug 30, 2016

why did you need to make this change?

@parkr, I'll try to explain things to the best of my ability.
The main reason why you are unable to understand this is because you've personally not tested it out on a windows-machine per se. So I'll include references to Appveyor logs that you can verify.

value $(pwd) _before_ $0: creating new default site == _/c/projects/jekyll_

say, I move assigning to $(pwd) further down (after pushd temp/default-site), then
$(pwd) == _/c/projects/jekyll/tmp/default-site_ ( An incorrect path on Windows )

But.. when I assign workdir to simply be two directories up ( _../../_ ) _after_ pushd tmp/default-site, then
in-theory, its a place where one can expect to find Jekyll gemspec, on any platform.

Rightly so, Both Appveyor and Travis exits green, after executing every line.

This explained why I changed the value of workdir


Refs:
Failed test in Appveyor: https://ci.appveyor.com/project/jekyll/jekyll/build/386/job/vyjf304g5ovbkogn
Passed test in Appveyor: https://ci.appveyor.com/project/jekyll/jekyll/build/403/job/ryc5t0pkc9egovox

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 30, 2016

Member

Now to explain the change in bundle exec ... ... line in this script and the very reason for this PR:

on Windows bundle exec exe/jekyll new ... fails as exe/jekyll is not identified as an executable. As of this writing, a patch has been applied to run bundle exec ruby exe/jekyll new ... instead. And it works! So, excellent. But..

my enquiry here is : is having to run _exe/jekyll executable_ really necessary for this script?

CI uses Jekyll at source . for every build. I tested to see if that source is sufficient to run this script.
Both Travis & Appveyor says, they're cool with executing bundle exec jekyll new tmp/default-site using Jekyll cloned at source . (They both passed this test).
Now, I need a human perspective to that observation.

Member

ashmaroli commented Aug 30, 2016

Now to explain the change in bundle exec ... ... line in this script and the very reason for this PR:

on Windows bundle exec exe/jekyll new ... fails as exe/jekyll is not identified as an executable. As of this writing, a patch has been applied to run bundle exec ruby exe/jekyll new ... instead. And it works! So, excellent. But..

my enquiry here is : is having to run _exe/jekyll executable_ really necessary for this script?

CI uses Jekyll at source . for every build. I tested to see if that source is sufficient to run this script.
Both Travis & Appveyor says, they're cool with executing bundle exec jekyll new tmp/default-site using Jekyll cloned at source . (They both passed this test).
Now, I need a human perspective to that observation.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 30, 2016

Member

Referencing CI Build Jobs as of latest commit 53f0404 ( before I remove merge-conflicts and rebase )

_Travis CI:_ https://travis-ci.org/jekyll/jekyll/jobs/155967689
_Appveyor CI:_ https://ci.appveyor.com/project/jekyll/jekyll/build/403/job/ryc5t0pkc9egovox

Member

ashmaroli commented Aug 30, 2016

Referencing CI Build Jobs as of latest commit 53f0404 ( before I remove merge-conflicts and rebase )

_Travis CI:_ https://travis-ci.org/jekyll/jekyll/jobs/155967689
_Appveyor CI:_ https://ci.appveyor.com/project/jekyll/jekyll/build/403/job/ryc5t0pkc9egovox

ashmaroli added some commits Aug 30, 2016

script/default-site
pushd tmp/default-site
+workdir="../../"
echo "$0: respecifying the jekyll install location"
ruby -e "contents = File.read('Gemfile'); File.write('Gemfile', contents.sub(/gem \"jekyll\".*\\n/, 'gem \"jekyll\", path: \"$workdir\"'))"

This comment has been minimized.

@parkr

parkr Aug 30, 2016

Member

Instead of using $workdir at all, what about just changing this line to use path: \"../../\"?

@parkr

parkr Aug 30, 2016

Member

Instead of using $workdir at all, what about just changing this line to use path: \"../../\"?

This comment has been minimized.

@ashmaroli

ashmaroli Aug 30, 2016

Member

Essentially, the same thing, but much better.
Removes the variable and shortens the line here by 2 characters 😜

I'll update shortly.

@ashmaroli

ashmaroli Aug 30, 2016

Member

Essentially, the same thing, but much better.
Removes the variable and shortens the line here by 2 characters 😜

I'll update shortly.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 30, 2016

Member

Seems to work! Thanks. LGTM.

Member

parkr commented Aug 30, 2016

Seems to work! Thanks. LGTM.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 30, 2016

Contributor

LGTM.

Contributor

envygeeks commented Aug 30, 2016

LGTM.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 30, 2016

Member

@jekyllbot: merge +dev

Member

parkr commented Aug 30, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 7c8dd4a into jekyll:master Aug 30, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jekyll/lgtm Approved by @parkr and @envygeeks.

jekyllbot added a commit that referenced this pull request Aug 30, 2016

@ashmaroli ashmaroli deleted the ashmaroli:script-default-site branch Aug 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment