asciidoctor: include the asciidoctor-epub3 gem #64298

michaelpj · 2019-07-04T12:21:36Z

Motivation for this change

This is another common output format for asciidoctor, and this gem is at least semi-official (maintained by the asciidoctor maintainer).

Things done

The first commit does some cleaning, by allowing us to use the default gem config in nixpkgs. We need this in the second commit as it makes nokogiri work. We could combine the configs properly, but it seems better to put our configuration for the mathematical gem centrally in case other people need to use it.

I also had to change how the update script works, but I think it's reasonable still.

The second commit actually adds the asciidoctor-epub3 gem. I've tested that this does actually work thereafter.

cc @worldofpeace @danbst who've looked at asciidoctor changes recently.

This has two benefits: - Anyone else using the `mathematical` gem will benefit from the config. - The gem config overrides the default one, so we were losing the fixes for other gems. I had to change how the update script works. Now it looks at the `gems` passthru from `bundlerApp`.

This is another common output target for asciidctor that requires an additional gem. The previous commit is necessary for this to work, so that we get the gem config for nokogiri correctly.

michaelpj · 2019-07-04T12:22:23Z

pkgs/tools/typesetting/asciidoctor/update.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
-rm gemset.nix Gemfile.lock


If we do this before building the shell then we can't build the shell!

Agreed! The more general issue is that the updater script is depending on the project to be working. So as soon as it breaks it becomes harder to repair it.

I don't see a way to avoid this while still piggybacking off the app definition to get all the dependencies....

🤦‍♂️ probably I was running a nix-shell inside nix-shell... I wish there was bundix --force-update

pkgs/tools/typesetting/asciidoctor/default.nix

zimbatm · 2019-07-04T12:32:18Z

pkgs/tools/typesetting/asciidoctor/update.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
-rm gemset.nix Gemfile.lock


Agreed! The more general issue is that the updater script is depending on the project to be working. So as soon as it breaks it becomes harder to repair it.

zimbatm · 2019-07-04T12:36:20Z

pkgs/tools/typesetting/asciidoctor/default.nix

  };
-}
+in app.overrideAttrs (attrs: { passthru = attrs.passthru // { updateShell = shell; }; })


Suggested change

in app.overrideAttrs (attrs: { passthru = attrs.passthru // { updateShell = shell; }; })

in app

attach the shell directly on the app.passthru to avoid the overhead

This doesn't work as app.gems isn't defined then.

I pushed f726160 which demonstrates how to implement it. The key is that nix is a lazily-evaluated language.

pkgs/tools/typesetting/asciidoctor/default.nix

michaelpj · 2019-07-04T12:40:44Z

Also I've realised that I may want to include the epubcheck and kindlegen gems, since they don't seem to be properly included by default. Some bundler fanciness in the asciidoctor Gemfile. Investigating....

michaelpj · 2019-07-04T12:56:12Z

Comments addressed.

I've opted not to include the epubcheck and kindlegen dependencies. They're a bit dodgy, e.g. kindlegen just curls the binary from the internet and provides it. Even better, the paths to the epubcheck and kindlegen tools can just be given by environment variables, so there's no need to use the dodgy gems.

michaelpj · 2019-07-04T13:12:11Z

Actually, given the environment variables work, I can just wrap the executable. The remaining annoyance is that kindlegen is unfree. I guess I could gate that behind withKindlegen? What's the usual approach for optional unfree deps?

kindlegen is unfree, so we don't include it by default. The derivation can be overridden to include it.

michaelpj · 2019-07-04T14:09:44Z

Okay, I wrapped the executable and that works nicely. I've put something in so epubcheck and kindlegen are optional, with kindlegen being off by default due to being unfree. But if there's a more standard way, happy to use that.

zimbatm · 2019-07-04T16:10:44Z

pkgs/tools/typesetting/asciidoctor/default.nix

  };
-}
+in app.overrideAttrs (attrs: { passthru = attrs.passthru // { updateShell = shell; }; })


I pushed f726160 which demonstrates how to implement it. The key is that nix is a lazily-evaluated language.

michaelpj · 2019-07-04T16:22:57Z

Nice, thanks.

zimbatm · 2019-07-04T19:54:46Z

@michaelpj let me know if you agree with my changes, if yes I will merge the PR.

michaelpj · 2019-07-04T19:58:52Z

Yes, they're good!

lopsided98 · 2019-07-17T01:53:31Z

This had the effect of breaking the default NixOS configuration on armv6l and armv7l, because it pulls Java into the build-time closure through epubcheck. Java has been broken on these platforms for quite a while, and this PR simply made the issue obvious.

On the other hand, this is the only usage of Java in a basic NixOS system, and pulls in many other dependencies (at build time, so binary cache users are not affected), so I am wondering if we want to disable epubcheck by default. I can't decide whether this is a good idea, or if we are better having a more fully featured package. Is there any specific use-case that needs epubcheck?

michaelpj · 2019-07-17T09:36:32Z

Hm, I'd assumed that Java would be an unproblematic package! I also didn't know the default configuration depended on this (why? the manual?).

I think disabling epubcheck would be fine. Alternatively, we could override it to disable epubcheck at the use site which is problematic, e.g. where the manual uses it if that's the problem.

lopsided98 · 2019-07-18T00:49:06Z

The dependency chain is: udisks -> libblockdev -> libndctl -> asciidoctor

zimbatm · 2019-07-18T07:45:46Z

@michaelpj: can you send a PR to disable epubcheck please?

michaelpj · 2019-07-18T09:54:48Z

@zimbatm #65018

lopsided98 · 2019-07-18T11:41:35Z

I'm working on fixing Java on ARM as well, and should have a PR by this weekend.

michaelpj added 2 commits July 4, 2019 13:16

asciidctor: include the asciidoctor-epub3 gem

4224920

This is another common output target for asciidctor that requires an additional gem. The previous commit is necessary for this to work, so that we get the gem config for nokogiri correctly.

michaelpj requested review from alyssais and zimbatm as code owners July 4, 2019 12:21

michaelpj commented Jul 4, 2019

View reviewed changes

ofborg bot added 6.topic: ruby 8.has: documentation 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Jul 4, 2019

zimbatm reviewed Jul 4, 2019

View reviewed changes

asciidoctor: address review comments

8559363

asciidoctor: wrap asciidoctor-epub3 for epubcheck and kindlegen

d6b7b2a

kindlegen is unfree, so we don't include it by default. The derivation can be overridden to include it.

asciidoctor: fix update.sh

f726160

zimbatm approved these changes Jul 4, 2019

View reviewed changes

danbst approved these changes Jul 4, 2019

View reviewed changes

zimbatm merged commit a0f5348 into NixOS:master Jul 4, 2019

michaelpj mentioned this pull request Jul 18, 2019

asciidoctor: disable epubcheck by default #65018

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asciidoctor: include the asciidoctor-epub3 gem #64298

asciidoctor: include the asciidoctor-epub3 gem #64298

michaelpj commented Jul 4, 2019

michaelpj Jul 4, 2019

zimbatm Jul 4, 2019

michaelpj Jul 4, 2019

danbst Jul 4, 2019

zimbatm Jul 4, 2019

zimbatm Jul 4, 2019

michaelpj Jul 4, 2019

zimbatm Jul 4, 2019

michaelpj commented Jul 4, 2019

michaelpj commented Jul 4, 2019

michaelpj commented Jul 4, 2019

michaelpj commented Jul 4, 2019

zimbatm Jul 4, 2019

michaelpj commented Jul 4, 2019

zimbatm commented Jul 4, 2019

michaelpj commented Jul 4, 2019

lopsided98 commented Jul 17, 2019

michaelpj commented Jul 17, 2019

lopsided98 commented Jul 18, 2019

zimbatm commented Jul 18, 2019

michaelpj commented Jul 18, 2019

lopsided98 commented Jul 18, 2019

		@@ -1,6 +1,6 @@
		#!/usr/bin/env bash
		rm gemset.nix Gemfile.lock

	in app.overrideAttrs (attrs: { passthru = attrs.passthru // { updateShell = shell; }; })
	in app

asciidoctor: include the asciidoctor-epub3 gem #64298

asciidoctor: include the asciidoctor-epub3 gem #64298

Conversation

michaelpj commented Jul 4, 2019

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Jul 4, 2019

michaelpj commented Jul 4, 2019

michaelpj commented Jul 4, 2019

michaelpj commented Jul 4, 2019

Choose a reason for hiding this comment

michaelpj commented Jul 4, 2019

zimbatm commented Jul 4, 2019

michaelpj commented Jul 4, 2019

lopsided98 commented Jul 17, 2019

michaelpj commented Jul 17, 2019

lopsided98 commented Jul 18, 2019

zimbatm commented Jul 18, 2019

michaelpj commented Jul 18, 2019

lopsided98 commented Jul 18, 2019