Skip to content
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

jekyll: 3.0.1 -> 3.4.1 #23471

Merged
merged 4 commits into from Apr 30, 2017
Merged

jekyll: 3.0.1 -> 3.4.1 #23471

merged 4 commits into from Apr 30, 2017

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Mar 4, 2017

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@veprbl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pesterhazy, @jgillich and @bdimcheff to be potential reviewers.

@veprbl
Copy link
Member Author

veprbl commented Mar 4, 2017

Ah, I guess, this is broken:

$ nix-shell -p jekyll --command "jekyll new test"  
Running bundle install in /private/tmp/test... 
  Bundler: There was an error while trying to write to
  Bundler: `/nix/store/l67429rhvrmr7c4c1msb7s8zjq4fx7ad-gemfile-and-lockfile/.bundle/config`.
  Bundler: It is likely that you need to grant write permissions for that path.

jekyll = attrs: {
postInstall = ''
installPath=$(cat $out/nix-support/gem-meta/install-path)
sed -i $installPath/lib/jekyll/commands/new.rb -e 's@Exec.run("bundle", "install"@Exec.run("nix-shell", "-p", "bundler", "--pure", "--run", "${bundler}/bin/bundle install"@'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's clever. I suppose there's no problem with nested nix-shell environments? (I never tried, AFAIK.)

Copy link
Member Author

@veprbl veprbl Mar 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@league I think this exec should be disabled altogether. Nothing that user can specify in the generated gemfile will propagate to nix'ed jekyll. User would have to add stuff to override pkgs/applications/misc/jekyll/Gemfile . What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example user in #20617 needs an additional Gem and there is no straightforward way to add it.

@veprbl
Copy link
Member Author

veprbl commented Mar 4, 2017

Basic functionality works in my tests now. There is still a problem with building sites that depend on additional plugins (e.g. #20617), but it is there already and this PR has nothing to do with it.

Original error:

$ nix-shell -p jekyll --command "jekyll new test"
Running bundle install in /private/tmp/test...
  Bundler: There was an error while trying to write to
  Bundler:
`/nix/store/l67429rhvrmr7c4c1msb7s8zjq4fx7ad-gemfile-and-lockfile/.bundle/config`.
  Bundler: It is likely that you need to grant write permissions for
that path.
@league
Copy link
Contributor

league commented Mar 4, 2017

LGTM – I don't immediately see a better way to handle the bundler issue. The fact that jekyll new now generates its own Gemfile makes things a little complicated for NixOS – but at least with this solution the basic tutorials for building a new site with default plugins will work. Maybe one day, commonly-used plugins could be optional arguments to the nix expression…?

@league
Copy link
Contributor

league commented Mar 4, 2017

@veprbl Now that I see how one patches up the installed Ruby code, this small addition to gem-config would also address #22858… but either way, I'll see how far I get upstream.

  # disable bundle install as it can't install anything in addition to what is
  # specified in pkgs/applications/misc/jekyll/Gemfile anyway. Also do chmod_R
  # to compensate for read-only files in site_template in nix store.
  jekyll = attrs: {
    postInstall = ''
      installPath=$(cat $out/nix-support/gem-meta/install-path)
      sed -i $installPath/lib/jekyll/commands/new.rb \
          -e 's@Exec.run("bundle", "install"@Exec.run("true"@' \
          -e 's@FileUtils.cp_r site_template + "/.", path@FileUtils.cp_r site_template + "/.", path; FileUtils.chmod_R "u+w", path@'
    '';
  };

@veprbl
Copy link
Member Author

veprbl commented Mar 4, 2017

@league cp_r has :preserve argument. Can't find docs for it though.

@veprbl
Copy link
Member Author

veprbl commented Mar 4, 2017

Seems to not be able to do what we want. The problem seems to be a standard one: how to get files at 0644 and directories at 0755. I don't know if this can be done beautifully enough to upstream. Might have to stick with patching in nix.

@league
Copy link
Contributor

league commented Mar 4, 2017

@veprbl Right. I also read (not under cp_r doc, but for copy_entry) that "If preserve is true, this method preserves owner, group, and modified time. Permissions are copied regardless preserve." https://ruby-doc.org/stdlib-2.2.2/libdoc/fileutils/rdoc/FileUtils.html

@veprbl
Copy link
Member Author

veprbl commented Mar 4, 2017

This calls for a ruby wizard :)

@veprbl
Copy link
Member Author

veprbl commented Mar 28, 2017

this is good to be merged btw

@7c6f434c 7c6f434c merged commit 4540123 into NixOS:master Apr 30, 2017
@veprbl veprbl deleted the jekyll branch December 1, 2020 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants