Skip to content

Comments

asciidoctor: Init at 1.5.4#15135

Merged
zimbatm merged 2 commits intoNixOS:masterfrom
yacinehmito:asciidoctor
May 4, 2016
Merged

asciidoctor: Init at 1.5.4#15135
zimbatm merged 2 commits intoNixOS:masterfrom
yacinehmito:asciidoctor

Conversation

@yacinehmito
Copy link
Contributor

@yacinehmito yacinehmito commented May 1, 2016

This was sitting in my personal repo until @aneeshusa suggested that I share it.

Asciidoctor is packaged with some of its optional dependencies, this is why there are both asciidoctor and asciidoctor-full as pkgs attributes names. I would very much like the help of someone versed in packaging gems to make this configurable.

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
    • OS X
    • 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

By analyzing the blame information on this pull request, we identified @zimbatm, @edolstra and @vcunat to be potential reviewers

@edolstra
Copy link
Member

edolstra commented May 2, 2016

The main question for whether to have a full version is whether the optional dependencies are very large (e.g. an optional dependency on Qt would count), and whether there are users who might want to avoid those dependencies (e.g. Git has an optional X11 dependency, but it's perfectly usable without, and there are scenarios like containers where you might want to have a small Git package). If either are not the case then it's probably better to drop the full version.

@zimbatm
Copy link
Member

zimbatm commented May 2, 2016

Just including asciidoctor-diagram and bespoke in the main asciidoctor package is fine. Other than that the packaging looks good to me.

@yacinehmito
Copy link
Contributor Author

yacinehmito commented May 2, 2016

I followed your suggestion and removed the asciidoctor-full attribute in favor to asciidoctor.
I also took the liberty to add the pdf and latex extensions. I don't use them but they are both popular, and somewhat essential to cover all the use cases.

Also, I would be grateful if you could look at how to prevent bundlerEnv from creating unwanted executables. Currently, one is created in the bin folder for every dependency, which is unneeded.

@zimbatm
Copy link
Member

zimbatm commented May 2, 2016

Alright, take a look at pkgs/development/tools/continuous-integration/cide/default.nix to see how to only expose select binaries.

Copy link
Member

Choose a reason for hiding this comment

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

No need to say that. A lot of different things could happen in the future :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing right now.

@yacinehmito
Copy link
Contributor Author

Thanks for the tip.

If I understand well, the derivation exposes a shell script that will call the given executable with the specified environment. This should do the trick but it adds a layer when none is needed. The environment of the executables are fine. I guess I could just rm the binaries I don't want.

@zimbatm
Copy link
Member

zimbatm commented May 2, 2016

Sure, removing the unecessary bin scripts in a postInstall step would also work. Ideally with a whitelist do new binaries don't emerge when upgrading the packages.

@joachifm
Copy link
Contributor

joachifm commented May 4, 2016

Is this ready?

@zimbatm
Copy link
Member

zimbatm commented May 4, 2016

It's almost ready, it just needs a bit of work to remove the unnecessary binaries that are being produced by the bundlerEnv.

@yacinehmito
Copy link
Contributor Author

I tried to add a whitelist attribute to bundlerEnv but finally decided to go the ugly route and delete manually the unneeded executables. It's much simpler and doesn't require to list the extensions.

@zimbatm zimbatm merged commit 0bc713f into NixOS:master May 4, 2016
@zimbatm
Copy link
Member

zimbatm commented May 4, 2016

perfect, thanks !

@aneeshusa
Copy link
Contributor

This isn't working for me, it can't find a mirror for asciidoctor-latex:

error: cannot download asciidoctor-latex-1.5.0.6.dev.gem from any mirror
builder for ‘/nix/store/0jfnhv9s40knjy07mab90sb2cl0z9dvn-asciidoctor-latex-1.5.0.6.dev.gem.drv’ failed with exit code 1

The URL returns a 404 as well.

@zimbatm
Copy link
Member

zimbatm commented May 5, 2016

I think this particular version has been yanked. https://rubygems.org/gems/asciidoctor-latex/ doesn't list it either.
@gpyh mind making another PR to bump the version to 1.5.0.8.dev ?

yacinehmito added a commit to yacinehmito/nixpkgs that referenced this pull request May 5, 2016
asciidoctor-latex: 1.5.0.6.dev -> 1.5.0.8.dev
See NixOS/pull/15135#issuecomment-216984749
This optional dependency may be removed if this happens too often
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants