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

Allow static files to be symlinked in unsafe mode or non-prod environments #4640

Merged
merged 4 commits into from Mar 25, 2016

Conversation

Projects
None yet
4 participants
@surrim

surrim commented Mar 6, 2016

This patch enables symlinks for static files. Now you can do something like this:

news/
    new-pc.jpg -> ../images/DSC0001.jpg (21 bytes)
    _posts/
        2016-03-06-new-pc.md
images/
    DSC0001.jpg (1,2 MiB)

The generated page could be something like that:

_site/
    news/
        2016-03-06-new-pc.html
        new-pc.jpg -> ../images/DSC0001.jpg (21 bytes, 1,2 MiB with jekyll 3.1.2)
    images/
        DSC0001.jpg (1,2 MiB)
  • no need to grep for invalid URLs (e.g. DSC0001.jpg becomes dsc0001.jpg), written things can stay untouched when filenames or folders change
  • detecting broken symlinks is easy (file explorer, ls -l, etc.)
  • nice hyperlinks for visitors (/news/new-pc.jpg) without grown webspace requirements and reduced jekyll build time

@parkr parkr added the undetermined label Mar 9, 2016

Show outdated Hide outdated lib/jekyll/static_file.rb
@@ -80,7 +80,7 @@ def write(dest)
FileUtils.mkdir_p(File.dirname(dest_path))
FileUtils.rm(dest_path) if File.exist?(dest_path)
FileUtils.cp(path, dest_path)
FileUtils.copy_entry(path, dest_path)

This comment has been minimized.

@parkr

parkr Mar 9, 2016

Member

Could you please wrap this around a @site.safe call? We cannot use copy_entry in safe mode.

if @site.safe || Jekyll.env.start_with?("prod")
  FileUtils.cp(path, dest_path)
else
  FileUtils.copy_entry(path, dest_path)
end

The Jekyll.env.start_with? "prod" ensures production builds (where the destination and source may not be on the same host) work as expected.

@parkr

parkr Mar 9, 2016

Member

Could you please wrap this around a @site.safe call? We cannot use copy_entry in safe mode.

if @site.safe || Jekyll.env.start_with?("prod")
  FileUtils.cp(path, dest_path)
else
  FileUtils.copy_entry(path, dest_path)
end

The Jekyll.env.start_with? "prod" ensures production builds (where the destination and source may not be on the same host) work as expected.

@parkr parkr changed the title from enable symlinks for static files to Allow static files to be symlinked in unsafe mode or non-prod environments Mar 9, 2016

@surrim

This comment has been minimized.

Show comment
Hide comment
@surrim

surrim Mar 11, 2016

I just updated the code and tested it, works fine here. Thanks for your assistance.

surrim commented Mar 11, 2016

I just updated the code and tested it, works fine here. Thanks for your assistance.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 11, 2016

Please just == production We shouldn't allow any derivative thereof of the word production because that just complicates everything in the long run. However, that said, I think this is fallible anyways, if the user disables safe in production it should count.

Please just == production We shouldn't allow any derivative thereof of the word production because that just complicates everything in the long run. However, that said, I think this is fallible anyways, if the user disables safe in production it should count.

This comment has been minimized.

Show comment
Hide comment
@surrim

surrim Mar 11, 2016

Owner

So better use @site.safe as the only condition?

Owner

surrim replied Mar 11, 2016

So better use @site.safe as the only condition?

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 11, 2016

IMO yes, because afaiw Github already enforces safe, as we do on some of our production builds here where I am. /cc @benbalter @parkr for confirmation.

envygeeks replied Mar 11, 2016

IMO yes, because afaiw Github already enforces safe, as we do on some of our production builds here where I am. /cc @benbalter @parkr for confirmation.

This comment has been minimized.

Show comment
Hide comment
@surrim

surrim Mar 11, 2016

Owner

Fixed again.

Owner

surrim replied Mar 11, 2016

Fixed again.

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 11, 2016

@surrim @envygeeks I think doing either safe OR production makes the most sense because not all production builds use the safe flag (e.g. if they want to use _plugins) but may still want other production-like outputs (e.g. their CSS/JS minified). In a production environment, we should consider the destination and source independent of each other, whereas only relying on safe would cause on non-safe production build to fail if you changed/updated/removed the source.

parkr replied Mar 11, 2016

@surrim @envygeeks I think doing either safe OR production makes the most sense because not all production builds use the safe flag (e.g. if they want to use _plugins) but may still want other production-like outputs (e.g. their CSS/JS minified). In a production environment, we should consider the destination and source independent of each other, whereas only relying on safe would cause on non-safe production build to fail if you changed/updated/removed the source.

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 11, 2016

This would be a good way to start enforcing that notion though. Before safe was a meh situation for all parties, now we enforce them to know what it is and how to use it, otherwise why have it there if it's only an internal feature?

envygeeks replied Mar 11, 2016

This would be a good way to start enforcing that notion though. Before safe was a meh situation for all parties, now we enforce them to know what it is and how to use it, otherwise why have it there if it's only an internal feature?

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 11, 2016

@envygeeks Safe mode and production aren't equivalent, though, and I don't think we should consider them equivalent.

parkr replied Mar 11, 2016

@envygeeks Safe mode and production aren't equivalent, though, and I don't think we should consider them equivalent.

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 11, 2016

I implied they weren't, therefore production shouldn't even be a factor in whether not to allow this feature to pass through. The only thing that matters to this feature is whether or not the site is safe or not. Whether or not it's production shouldn't matter one bit.

envygeeks replied Mar 11, 2016

I implied they weren't, therefore production shouldn't even be a factor in whether not to allow this feature to pass through. The only thing that matters to this feature is whether or not the site is safe or not. Whether or not it's production shouldn't matter one bit.

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 11, 2016

@envygeeks Oh, ok, that's where we disagree. When I make a production build with copy_entry instead of cp and the source is a symlink, I could very easily break my site if I modify the symlinked file. The point of a production build is to be standalone – copying a symlink breaks that. In production mode, there should be no symlinks.

parkr replied Mar 11, 2016

@envygeeks Oh, ok, that's where we disagree. When I make a production build with copy_entry instead of cp and the source is a symlink, I could very easily break my site if I modify the symlinked file. The point of a production build is to be standalone – copying a symlink breaks that. In production mode, there should be no symlinks.

This comment has been minimized.

Show comment
Hide comment
@surrim

surrim Mar 11, 2016

Owner

That's why I only use relative symlinks. There you don't have this problem.
I think everything linked inside the Jekyll directory should be okay. Symlinks outside like /usr/share/licenses/bash/COPYING should be copied in production mode.

Owner

surrim replied Mar 11, 2016

That's why I only use relative symlinks. There you don't have this problem.
I think everything linked inside the Jekyll directory should be okay. Symlinks outside like /usr/share/licenses/bash/COPYING should be copied in production mode.

surrim
Show outdated Hide outdated lib/jekyll/static_file.rb
@@ -80,7 +80,11 @@ def write(dest)
FileUtils.mkdir_p(File.dirname(dest_path))
FileUtils.rm(dest_path) if File.exist?(dest_path)
FileUtils.cp(path, dest_path)
if @site.safe

This comment has been minimized.

@parkr

parkr Mar 24, 2016

Member

I am happy to accept this if you add Jekyll.env == "production" here.

Production builds should produce an artifact (the compiled site) that is standalone and does not have any external dependencies on other files. A symlink cannot be guaranteed to validate that requirement.

@parkr

parkr Mar 24, 2016

Member

I am happy to accept this if you add Jekyll.env == "production" here.

Production builds should produce an artifact (the compiled site) that is standalone and does not have any external dependencies on other files. A symlink cannot be guaranteed to validate that requirement.

@surrim

This comment has been minimized.

Show comment
Hide comment
@surrim

surrim Mar 25, 2016

I added the condition again.
Could you please add some more documentation about safe mode? Maybe updating the configuration page with a GitHub hint would be enough.

surrim commented Mar 25, 2016

I added the condition again.
Could you please add some more documentation about safe mode? Maybe updating the configuration page with a GitHub hint would be enough.

@surrim surrim closed this Mar 25, 2016

@surrim surrim reopened this Mar 25, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 25, 2016

Member

@surrim Sounds good!

Member

parkr commented Mar 25, 2016

@surrim Sounds good!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 25, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Mar 25, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 1ab0ed3 into jekyll:master Mar 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Mar 25, 2016

parkr added a commit that referenced this pull request Mar 26, 2016

Merge remote-tracking branch 'origin/master' into themes
* origin/master: (65 commits)
  Update history to reflect merge of #4703 [ci skip]
  Update history to reflect merge of #4712 [ci skip]
  Highlight the test code
  Update history to reflect merge of #4640 [ci skip]
  readded "env=prod"-condition
  Update history to reflect merge of #3849 [ci skip]
  Update history to reflect merge of #4624 [ci skip]
  Update history to reflect merge of #4704 [ci skip]
  Update history to reflect merge of #4706 [ci skip]
  Checks for link file extension in tests
  Updating assets documentation
  Fix test teardown for cleaner.
  Update history to reflect merge of #4542 [ci skip]
  Add explanation of site variables in the example _config.yml
  Use double quotes in the gemfile
  Add test for creation of Gemfile by 'jekyll new'
  Add comment about github-pages
  Update history to reflect merge of #4533 [ci skip]
  Ensure Rouge closes its div/figure properly after highlighting ends.
  Add Site#config= which can be used to set the config
  ...

@DirtyF DirtyF referenced this pull request May 25, 2018

Open

Symbolic links are broken when being copied to _site #6870

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