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

hugo: helpers/content.go: call rst2html directly to render content #47600

Merged
merged 1 commit into from Oct 13, 2018

Conversation

Projects
None yet
3 participants
@shreyanshk
Copy link
Contributor

shreyanshk commented Oct 1, 2018

Motivation for this change

Hugo launches rst2html by calling Python interpreter and passing it the path of rst2html file.
This doesn't work on NixOS because the real rst2html is actually wrapped behind a bash script which is responsible for correctly setting the env and then exec-ing the real rst2html.

The problem is that rst2html is actually a bash script and not a Python script. Hence, Python fails to launch it with SyntaxError.

This patch ensures that rst2html is called directly so the shebang work correctly.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.
pkgs/applications/misc/hugo/010-call-rst2html-directly.patch Outdated
}

-func getPythonExecPath() string {
- path, err := exec.LookPath("python")

This comment has been minimized.

@Mic92

Mic92 Oct 1, 2018

Contributor

This could be even incorrect if python is not the correct version python version for rst2html. Can you also open an issue upstream? They really only should do this on windows in my opinion and even there this probably also will work if rst2html is associated with the correct extension (i.e. rst2html.py).

This comment has been minimized.

@Mic92

Mic92 Oct 1, 2018

Contributor

Also they check if rst2html is in $PATH and is executable. Is this even true on windows?

This comment has been minimized.

@Mic92

Mic92 Oct 1, 2018

Contributor

After digging in LookPath on windows it appears that .py has to be in PATHEXT to be found by LookPath. This means if it is not already a registered extension it would not be found anyway. If it is registered then prepending python should be not necessary.

This comment has been minimized.

@shreyanshk

shreyanshk Oct 2, 2018

Author Contributor

I know. I am unable to follow their reasoning as well.
I opened an issue upstream like a month ago but haven't received any response at all.
I think it's maybe because Nix is not on their priority list as they have quite a many other issues open. So, maybe we can apply the patch ourselves.

Of all the platforms where Nix works, except Windows, all natively support shebangs.
And, on Windows, Nix works under WSL (experimental) which supports shebangs. So, I believe, we can directly call rst2html for our use case without any issues.

This comment has been minimized.

@Mic92

This comment has been minimized.

@Mic92

Mic92 Oct 3, 2018

Contributor

Do you want to wait a few days/hours for them to merge or do you want to go ahead with the current version?

This comment has been minimized.

@shreyanshk

shreyanshk Oct 4, 2018

Author Contributor

Ideally, it should be merged. Hugo's next release is at least a month away.

This comment has been minimized.

@Mic92

Mic92 Oct 4, 2018

Contributor

The idea was to use fetchpatch instead when it is merged in hugo master.

This comment has been minimized.

@shreyanshk

shreyanshk Oct 5, 2018

Author Contributor

Oh! let's wait for upstream to merge it.

This comment has been minimized.

@Mic92

Mic92 Oct 5, 2018

Contributor

The community looks active hopefully this will not take long.

pkgs/applications/misc/hugo/default.nix Outdated
@@ -12,6 +12,9 @@ buildGoPackage rec {
rev = "v${version}";
sha256 = "0n27vyg66jfx4lwswsmdlybly8c9gy5rk7yhy7wzs3rwzlqv1jzj";
};
patches = [
./010-call-rst2html-directly.patch

This comment has been minimized.

@Mic92

Mic92 Oct 12, 2018

Contributor

Ok. Now this can fetched from the upstream repository.

hugo: apply upstream patch to call rst2html directly on Nix
Initially, rst2html was called via the python interpreter which would
fail if the script was wrapped in a launcher as on NixOS.
pkgs/applications/misc/hugo/rst.nix Outdated
{ stdenv, hugo, python3, which }:

stdenv.mkDerivation rec {
name = "hugoWithRST";

This comment has been minimized.

@Mic92

Mic92 Oct 12, 2018

Contributor

Do we really need this as a separate package? All it does is adding pygments. Users who need hugo + docutils + pygments can just install both or write a shell.nix for their project.

This comment has been minimized.

@shreyanshk

shreyanshk Oct 12, 2018

Author Contributor

I tried. Installing both packages doesn't work. Hugo is able to find docutils but not pygments.
What I did was:

nix-env -iA nixpkgs.hugo
nix-env -iA nixpkgs.python36Packages.docutils
nix-env -iA nixpkgs.python36Packages.pygments

But, that resulted in hugo reporting that it couldn't find pygments.

It looks like docutils needs pygments and it is unable to find it even if both are installed (but separately) because pygments is not on docutils's PATH (or PYTHONPATH).

Additionally, sure shell.nix works but then shell customization are lost. So, I added it.

This comment has been minimized.

@Mic92

Mic92 Oct 13, 2018

Contributor

I usually use direnv to preserve my shell: https://github.com/direnv/direnv/wiki/Nix

This comment has been minimized.

@Mic92

Mic92 Oct 13, 2018

Contributor

It could be made a build option of hugo or a wrapper like weechat, but I would not clutter all-packages.nix with all possible dependency combinations.

This comment has been minimized.

@shreyanshk

shreyanshk Oct 13, 2018

Author Contributor

This approach is certainly simpler. I'll remove the extra package. Thank you.

This comment has been minimized.

@shreyanshk

shreyanshk Oct 13, 2018

Author Contributor

@Mic92 , I've updated the PR. Thanks.

@Mic92 Mic92 merged commit 2993047 into NixOS:master Oct 13, 2018

8 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
@shreyanshk

This comment has been minimized.

Copy link
Contributor Author

shreyanshk commented Oct 18, 2018

@Mic92 ,
When would this patch be added into the nixos-18.09 channel? I see the channel was updated some time ago but the patch is still not present into that branch.
Thanks.

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Oct 18, 2018

I have not backported the commit back then to 18.09. This is now the case: f8847fc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.