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

Do not coerce layout paths in theme-gem to the source directory #6603

Merged
merged 6 commits into from Jan 2, 2018

Conversation

@ashmaroli
Member

ashmaroli commented Dec 6, 2017

Resolves #6241
Resolves #6332

Do not coerce layout paths in theme-gem to the source directory.

/cc @jekyll/build

@ashmaroli ashmaroli added the fix label Dec 6, 2017

@ashmaroli ashmaroli added this to the v3.7.0 milestone Dec 6, 2017

@jekyllbot jekyllbot requested review from ayastreb, mattr- and parkr Dec 6, 2017

@ashmaroli ashmaroli referenced this pull request Dec 6, 2017

Closed

incremental build fails for very basic example #6332

5 of 18 tasks complete
Show outdated Hide outdated features/incremental_rebuild.feature Outdated
Show outdated Hide outdated lib/jekyll/regenerator.rb Outdated
@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Dec 7, 2017

Member

I tried this branch with @mmistakes theme and the build time is way faster:

Before

Regeneration time is almost the same as the initial build time:

 Incremental build: enabled
      Generating...
                    done in 11.622 seconds.
 Auto-regeneration: enabled for '/Users/frank/code/jekyll/tests/minimal-mistakes/test'
    Server address: http://127.0.0.1:4000/test/
  Server running... press ctrl-c to stop.
      Regenerating: 1 file(s) changed at 2017-12-15 11:19:48
                    _portfolio/baz-boom-identity.md
                    ...done in 11.151307 seconds.

After

                    done in 14.231 seconds.
 Auto-regeneration: enabled for '/Users/frank/code/jekyll/tests/minimal-mistakes/test'
    Server address: http://127.0.0.1:4000/test/
  Server running... press ctrl-c to stop.
      Regenerating: 1 file(s) changed at 2017-12-15 11:22:22
                    _portfolio/baz-boom-identity.md
                    ...done in 2.390946 seconds.
Member

DirtyF commented Dec 7, 2017

I tried this branch with @mmistakes theme and the build time is way faster:

Before

Regeneration time is almost the same as the initial build time:

 Incremental build: enabled
      Generating...
                    done in 11.622 seconds.
 Auto-regeneration: enabled for '/Users/frank/code/jekyll/tests/minimal-mistakes/test'
    Server address: http://127.0.0.1:4000/test/
  Server running... press ctrl-c to stop.
      Regenerating: 1 file(s) changed at 2017-12-15 11:19:48
                    _portfolio/baz-boom-identity.md
                    ...done in 11.151307 seconds.

After

                    done in 14.231 seconds.
 Auto-regeneration: enabled for '/Users/frank/code/jekyll/tests/minimal-mistakes/test'
    Server address: http://127.0.0.1:4000/test/
  Server running... press ctrl-c to stop.
      Regenerating: 1 file(s) changed at 2017-12-15 11:22:22
                    _portfolio/baz-boom-identity.md
                    ...done in 2.390946 seconds.

@DirtyF DirtyF requested a review from jekyll/build Dec 15, 2017

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Dec 15, 2017

Member

CI Tests are passing, a quick benchmark shows a significant improvement (should benefit all jekyll-remote-themeusers), is there anything left blockin' this?

/cc @benbalter

Member

DirtyF commented Dec 15, 2017

CI Tests are passing, a quick benchmark shows a significant improvement (should benefit all jekyll-remote-themeusers), is there anything left blockin' this?

/cc @benbalter

@parkr parkr referenced this pull request Dec 15, 2017

Closed

Release v3.7.0 #6587

3 of 3 tasks complete
@mattr-

This is looking 👍. I have one small comment about security that could use an answer.

Show outdated Hide outdated lib/jekyll/renderer.rb Outdated
rendered documents should be within site source
while a layout object (a dependency) can point to a file within the source
directory or within the theme-gem, other renderable objects can be coerced
to within the source directory.

@ashmaroli ashmaroli changed the title from Incremental option should not regenerate theme-gem files to Incremental option should not regenerate theme-gem files [WIP] Dec 17, 2017

@ashmaroli ashmaroli changed the title from Incremental option should not regenerate theme-gem files [WIP] to Incremental option should not regenerate files that depend solely on files in the theme-gem Dec 17, 2017

@ashmaroli ashmaroli changed the title from Incremental option should not regenerate files that depend solely on files in the theme-gem to Do not coerce layout paths in theme-gem to the source directory Dec 17, 2017

@ashmaroli ashmaroli dismissed stale reviews from benbalter and DirtyF Dec 17, 2017

The patch changed considerably since last review

@DirtyF DirtyF requested review from jekyll/build and removed request for ayastreb Dec 17, 2017

@Crunch09

👍

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Jan 2, 2018

Member

one more review for this, and we can ship it in 3.7.0!

Member

oe commented Jan 2, 2018

one more review for this, and we can ship it in 3.7.0!

@DirtyF

DirtyF approved these changes Jan 2, 2018

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jan 2, 2018

Member

@jekyllbot: merge +minor

Member

DirtyF commented Jan 2, 2018

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 3c959af into jekyll:master Jan 2, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ashmaroli ashmaroli deleted the ashmaroli:no-regen-theme-files branch Jan 4, 2018

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