sile: 0.10.4 -> 0.10.9

[alerque]

Motivation for this change

Upstream release

Note that I am not running Nix, this is in the blind. Prior to this release make test was a heavyweight test suite that downloaded a bunch of required fonts for regression testing. As of this release there is now a lightweight make check that just makes sure we can generate a valid PDF file, no fonts are downloaded. It does require poppler (for pdfinfo) to run, so that may need to be added somewhere as a make dependency.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• 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 nixpkgs-review --run "nixpkgs-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)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[doronbehar]

Apply this diff on top of current changes to make CI happy:

diff --git i/pkgs/tools/typesetting/sile/default.nix w/pkgs/tools/typesetting/sile/default.nix
index 1a5c780798f..3cef706da71 100644
--- i/pkgs/tools/typesetting/sile/default.nix
+++ w/pkgs/tools/typesetting/sile/default.nix
@@ -1,5 +1,5 @@
 { stdenv, darwin, fetchurl, makeWrapper, pkgconfig, autoconf, automake
-, harfbuzz, icu
+, harfbuzz, icu, poppler
 , fontconfig, lua, libiconv
 , makeFontsConf, gentium
 }:
@@ -17,7 +17,7 @@ stdenv.mkDerivation rec {
 
   src = fetchurl {
     url = "https://github.com/sile-typesetter/sile/releases/download/v${version}/${pname}-${version}.tar.bz2";
-    sha256 = "a14fe23af242ba723aed699048b10abf60d1809390ac543140b80e057c4b4b9b";
+    sha256 = "16sb9dy0a3mq80qm9b4hjf0d2q5z1aqli439xlx75fj2y8xf4kx1";
   };
 
   configureFlags = [ "--with-system-luarocks" ];

[alerque]

Thanks for the tip @doronbehar. I did that and it helped, but I think there is still an issue. Even though the two tests that failed are somehow allowed to fail, they shouldn't have:

This is SILE v0.10.5
<STDIN>
[1]
/nix/store/ypag3bh7y7i15xf24zihr343wi6x5i6g-bash-4.4-p23/bin/bash: line 3: pdfinfo: command not found
make: *** [Makefile:1179: check] Error 1

If poppler was available we should have had pdfinfo which is used to check that the file generated by SILE is a valid PDF file (i.e. the whole stack is working from input to output).

[doronbehar]

That's because pdfinfo is part of `poppler_utils` and not `poppler`. So change that and we'll see then.

[alerque]

Now I'm not sure what I'm looking at. Are the two skips / failures still meaningful?

[doronbehar]

That error is due to a bad implementation of the way we split outputs to derivations. I once encountered similar frustrating errors that I had no idea where they came from or what line was the fault :/. Ideally though, such errors shouldn't arise. if we do try to enable split outputs, our build system passes to your configure script this flag:

--docdir=/nix/store/qxd6cvgya7y2458n9cz4dmqnp0hvbmr1-sile-0.10.5-doc/share/doc/sile

Which points to where the docs should eventually go into. I think your configure script accepts this flag right? After debugging a bit, I reached the conclusion that the CHANGELOG.md and README.md are indeed installed in where --docdir is pointing to, but sile.pdf isn't, and that's because of what's in postInstall. Speaking of splitting outputs, I'd split sile to even more outputs, although that the implementation is somewhat buggy. Anyway, here's a further change that works:

diff --git i/pkgs/tools/typesetting/sile/default.nix w/pkgs/tools/typesetting/sile/default.nix
index f003312493e..b01077258c2 100644
--- i/pkgs/tools/typesetting/sile/default.nix
+++ w/pkgs/tools/typesetting/sile/default.nix
@@ -55,13 +55,15 @@ stdenv.mkDerivation rec {
   checkTarget = "check";
 
   postInstall = ''
-    install -D -t $out/share/doc/sile documentation/sile.pdf
+    install -D -t $doc/share/doc/sile documentation/sile.pdf
   '';
 
   # Hack to avoid TMPDIR in RPATHs.
-  preFixup = ''rm -rf "$(pwd)" && mkdir "$(pwd)" '';
+  preFixup = ''
+    rm -rf "$(pwd)" && mkdir "$(pwd)"
+  '';
 
-  outputs = [ "out" "doc" ];
+  outputs = [ "out" "man" "dev" "doc" ];
 
   meta = {
     description = "A typesetting system";

Usually we don't have upstream maintainers involved downstream and I appreciate that. Noticing this package doesn't have any downstream NixOS maintainers, perhaps (in addition to the diff above):

diff --git i/pkgs/tools/typesetting/sile/default.nix w/pkgs/tools/typesetting/sile/default.nix
index b01077258c2..9514521e9ae 100644
--- i/pkgs/tools/typesetting/sile/default.nix
+++ w/pkgs/tools/typesetting/sile/default.nix
@@ -1,14 +1,38 @@
-{ stdenv, darwin, fetchurl, makeWrapper, pkgconfig, autoconf, automake
-, harfbuzz, icu, poppler_utils
-, fontconfig, lua, libiconv
-, makeFontsConf, gentium
+{ stdenv
+, darwin
+, fetchurl
+, makeWrapper
+, pkgconfig
+, autoconf
+, automake
+, harfbuzz
+, icu
+, poppler_utils
+, fontconfig
+, lua
+, libiconv
+, makeFontsConf
+, gentium
 }:
 
-with stdenv.lib;
-
 let
-  luaEnv = lua.withPackages(ps: with ps;[cassowary cosmo compat53 linenoise lpeg lua-zlib lua_cliargs luaepnf luaexpat luafilesystem luarepl luasec luasocket stdlib vstruct]);
-
+  luaEnv = lua.withPackages(ps: with ps; [
+    cassowary
+    cosmo
+    compat53
+    linenoise
+    lpeg
+    lua-zlib
+    lua_cliargs
+    luaepnf
+    luaexpat
+    luafilesystem
+    luarepl
+    luasec
+    luasocket
+    stdlib
+    vstruct
+  ]);
 in
 
 stdenv.mkDerivation rec {
@@ -20,19 +44,36 @@ stdenv.mkDerivation rec {
     sha256 = "16sb9dy0a3mq80qm9b4hjf0d2q5z1aqli439xlx75fj2y8xf4kx1";
   };
 
-  configureFlags = [ "--with-system-luarocks" ];
-
-  nativeBuildInputs = [ autoconf automake pkgconfig makeWrapper ];
-  buildInputs = [ harfbuzz icu fontconfig libiconv luaEnv ]
-  ++ optional stdenv.isDarwin darwin.apple_sdk.frameworks.AppKit
+  configureFlags = [
+    "--with-system-luarocks"
+  ];
+
+  nativeBuildInputs = [
+    autoconf
+    automake
+    pkgconfig
+    makeWrapper
+  ];
+  buildInputs = [
+    harfbuzz
+    icu
+    fontconfig
+    libiconv
+    luaEnv
+  ]
+    ++ stdenv.lib.optionals stdenv.isDarwin [
+      darwin.apple_sdk.frameworks.AppKit
+    ]
   ;
-  checkInputs = [ poppler_utils ];
+  checkInputs = [
+    poppler_utils
+  ];
 
-  preConfigure = optionalString stdenv.isDarwin ''
+  preConfigure = stdenv.lib.optionalString stdenv.isDarwin ''
     sed -i -e 's|@import AppKit;|#import <AppKit/AppKit.h>|' src/macfonts.m
   '';
 
-  NIX_LDFLAGS = optionalString stdenv.isDarwin "-framework AppKit";
+  NIX_LDFLAGS = stdenv.lib.optionalString stdenv.isDarwin "-framework AppKit";
 
   FONTCONFIG_FILE = makeFontsConf {
     fontDirectories = [
@@ -40,10 +81,7 @@ stdenv.mkDerivation rec {
     ];
   };
 
-  doCheck = true; /*stdenv.targetPlatform == stdenv.hostPlatform
-  && ! stdenv.isAarch64 # random seg. faults
-  && ! stdenv.isDarwin; # dy lib not found
- */
+  doCheck = true;
 
   enableParallelBuilding = true;
 
@@ -65,7 +103,7 @@ stdenv.mkDerivation rec {
 
   outputs = [ "out" "man" "dev" "doc" ];
 
-  meta = {
+  meta = with stdenv.lib; {
     description = "A typesetting system";
     longDescription = ''
       SILE is a typesetting system; its job is to produce beautiful

This diff is mostly about style, so I hope you wouldn't care but I'll explain why I think this diff is good:

1. Using more new lines to separate lists, future diffs will be easier to review.
2. Using with <something> is considered discouraged for large scope, so now it's only used in meta.

[alerque]

Which points to where the docs should eventually go into. I think your configure script accepts this flag right? After debugging a bit, I reached the conclusion that the CHANGELOG.md and README.md are indeed installed in where --docdir is pointing to, but sile.pdf isn't, and that's because of what's in postInstall.

You are correct, that should be fixed: sile/issues/919.

Anyway, here's a further change that works:

Applied.

Usually we don't have upstream maintainers involved downstream and I appreciate that. Noticing this package doesn't have any downstream NixOS maintainers, perhaps (in addition to the diff above):

Also applied, thanks.

By the way I squashed all that together in one commit, if you'd rather I broke it up by purpose I'd be happy to split it.

[alerque]

All green. Ping me if there is anything else I need to do.

[doronbehar]

/marvin opt-in

[alerque]

SILE v0.10.7 is out now and has one change that will affect Nix packaging. As documented in v0.10.6 release notes there are new flags for ./configure, namely --with-manual and --with-examples. Enabling them will cause make install to install those respective resources to the distro's $(docdir) (as reported by automake). This probably obsoletes the postInstall function in this package, but I'm not exactly sure how to proceed. I'm happy to add this update to this PR or open a new one, or let somebody else that knows what they are doing take care of the change.

[doronbehar]

You are an excellent developer. I have a few things to say:

1. I think --with-examples isn't necessary, and it's a bit hard to IMO to decide in what "output" to put them. Besides that, using --with-manual instead of that postInstall is a good idea, +1 on supplying this configure flag.

2. Our community is running a bot that automatically updates packages according to GitHub & GitLab releases, (for packages that are sourced from GitHub). It's called @r-ryantm and thanks to him, we are in the top chart here:

https://repology.org/repositories/statistics/newest

In order for sile to get updated automatically by the bot, (from experience it usually takes no more then a day or two), you need to use fetchFromGitHub which should look like this:

  src = fetchFromGitHub {
    owner = "sile-typesetter";
    repo = "sile";
    rev = "v${version}";
    sha256 = "1pba21ys6mv3fwxj8sri21biq8vkc58cnjjzmgr7pd1cb6scwad6";
  };

The subtlety here is that this fetches a tarball from the commit referenced as v${version} and not the release "tarball" that you post on your releases page. Using this fetch method fails for sile because of:

automake: error: cannot open < Makefile-distfiles: No such file or directory

I wish it was possible for you to change a bit your releases procedure so that pure git sources could be used to build sile.

3. Although you are not a NixOS user, we could still add you to the maintainers list, and then @r-ryantm should cc you when it opens the PR for the update. It will invite you to test the update, but naturally that won't be relevant though it may still benefit you to know the update is ready on our end. If you'd like to do that, you can add a commit to the PR with the text:

maintainers: add @alerque 

And add a line to sile's meta attribute (usually it's being put right below the homepage):

    maintainers = with maintainers; [ alerque ];

4. Even with the release tarball you have for 0.10.7, I notice you require automake1.15 while 1.16 is out:

/build/sile-0.10.7/missing: line 81: automake-1.15: command not found
WARNING: 'automake-1.15' is missing on your system.
         You should only need it if you modified 'Makefile.am' or ....

But that's no big deal, we could switch to automake1.15.

5. It's a bit unfortunate that you've had time to put out a new release and we still haven't merged this update here. I'm not a permitted to commit to Nixpkgs so I can't help a lot. There's a new bot we are testing now, it's called marvin-mk and it's purpose is to make PRs' management expressive and verbose. Anyway, don't mind much the details, the point is that we'll might be able to get some attention if you (If I'll do it it won't help, see this) will write in a comment:

/marvin opt-in

[alerque]

/marvin opt-in

[marvin-mk2]

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

[doronbehar]

/status awaiting_changes

[alerque]

1. I think `--with-examples` isn't necessary, and it's a bit hard to IMO to decide in what "output" to put them.

Fair enough. Not all distros will even want the manual. The package is a lot lighter without the examples, and installed somewhere on the system where you can't modify and play with them they aren't that much use anyway. They make much more sense to have in the release packaging for people that are playing with the source.

2. Our community is running a bot that automatically updates packages according to GitHub & GitLab releases, (for packages that are sourced from GitHub).

...
The subtlety here is that this fetches a tarball from the commit referenced as v${version} and not the release "tarball" that you post on your releases page. Using this fetch method fails for sile because of:

This is something you (Nix) should fix. There is tons of software out there that release source packages for a good reason! SILE‌ is hardly the only one in this situation. You actually can build SILE from the Github snapshot archive, but there are several disadvantages. To get it to work, you need to supply the version info:

echo $version > .tarball-version

Normally for Git clones that would be derived at configure time from git tag information or for source packages it is included in the package. It's impossible for the git zip archive that snapshots a commit to supply this information.

The disadvantage is that there is a lot more building you will have to do. The distribution source package (made using automake's make dist) includes some resources pre-compiled: notably relevant to Nix, the manual. You can build the manual, in fact it will happen automatically, but it will connect to the internet and download a bunch of fonts to do so.

That being said the source package file naming is consistent, it should be quite straight forward to make this a regular feature of fetchFromGitHub(), all you need is a file name pattern used by the project. In SILE's case it's sile-$version.tar.xz.

automake: error: cannot open < Makefile-distfiles: No such file or directory

This is because you didn't run the ./bootstrap.sh script. If you are trying to compile from a Git clone (or in your case the archive snapshot zip thingy) you need to run this before running configure.

I wish it was possible for you to change a bit your releases procedure so that pure git sources could be used to build sile.

Again, this is a catch 22 and a big deal for many software projects. Limiting yourself to git snapshots is not a good plan. You can work around this for SILE, but it's not the right solution.

1. Although you are not a NixOS user, we could still add you to the maintainers list, and then @r-ryantm should cc you when it opens the PR for the update.

Great, thanks!

1. Even with the release tarball you have for 0.10.7, I notice you require automake1.15 while 1.16 is out:

I don't think this is correct, you can use a newer automake. The bits of automake source in the package used to be derived from 1.16 because that's what I was building from, but the new fully automated releases being built on Github Actions have bits from 1.15 because that's what the latest builder image they are running has. But it doesn't actually matter, you can use 1.16 just fine. Depending on how strict your default flags for automake are you make need to run autoreconf.

[alerque]

How does one get the right checksum? It's not a regular SHA256 sum of the file.

[alerque]

@doronbehar I invited you as a collaborator to my fork of this repository so you should be able to commit to this branch/PR. Please make yourself at home! And thanks for all the tips to date. The whole process actually has me interested in giving Nix a spin!

[alerque]

@doronbehar I botched the automake install rules as they apply to the Docker image we build. The issue doesn't apply to the Nix build at all (it's a race condition while bundling all the Lua dependencies) but I'm going to be tagging yet another patch release, v0.10.8, to fix it soon. I don't think I'm wrong about the automake version stuff (i.e. I think you can build the source package without automake at all and from Git sources using automake1.16 if you want to) but if I'm wrong about that and there is something blocking it would be nice to fix before I tag another release.

[doronbehar]

@doronbehar I invited you as a collaborator to my fork of this repository

Thanks. I'll PR you there once I'll get something working.

Again, this is a catch 22 and a big deal for many software projects. Limiting yourself to git snapshots is not a good plan. You can work around this for SILE, but it's not the right solution.

We are not limited to pure git snapshots of course. Almost all standard GNU packages we ship are built from the releases that include a premade ./configure script. We also have hooks that enable building autoconf based projects by running either ./bootstrap.sh & friends. However, I tried pretty hard to use these scripts and to build sile straight from git, but it didn't work out. Hence I'd conclude it's not worth the effort. FYI, @r-ryantm also updates packages based on information from repology - meaning that it might be possible for you to only worry about updating sile for your distro alone, and it will then pick up the update based on information from there.

@doronbehar I botched the automake install rules as they apply to the Docker image we build. The issue doesn't apply to the Nix build at all (it's a race condition while bundling all the Lua dependencies) but I'm going to be tagging yet another patch release, v0.10.8, to fix it soon.

Great. I tried building 0.10.7 with automake 1.15 and interestingly it failed with:

WARNING: 'automake-1.15' is probably too old.
         You should only need it if you modified 'Makefile.am' or
         'configure.ac' or m4 files included by 'configure.ac'.
         The 'automake' program is part of the GNU Automake package:

Ideally, release tarball should not need automake & autoconf at all, but it's not big deal that you do. Feel free to decide what ever you want for 0.10.8 and to ping me to test it.

[alerque]

Thanks. I'll PR you there once I'll get something working.

You don't need to PR anything, you have write access to the branch (sile-0.10.5) this PR is reading from, you can just push directly too it.

We are not limited to pure git snapshots of course. Almost all standard GNU packages we ship are built from the releases that include a premade ./configure script. We also have hooks that enable building autoconf based projects by running either ./bootstrap.sh & friends. However, I tried pretty hard to use these scripts and to build sile straight from git, but it didn't work out.

I'm a little surprised to hear this. I build it from git all the time, our CI system builds it from git, other distro packages have been known to build it from git, etc. There shouldn't be too much to it.

1. First, you have to supply your own copy of the libtexpdf source tree. Since this is a Git module it's not part of the archive. Eventually this will be an external system library dependency checked by configure instead of just assuming it, we just haven't gotten there yet. For now it's distributed in the source build or you can get it yourself if you want to use the archive snapshots.

2. If by "pure git" you mean a the archive snapshot rather than a clone (i.e. no history) you also have to supply version information:

   echo 0.10.5 > .tarball-version
   

3. After that the same steps apply whether using a git clone or a snapshot archive:

   ./bootstrap.sh
   ./configure --with-system-luarocks
   make
   

   Note I do this on Arch Linux all the time which has automake-1.16.

I just added hints about all this stuff to the output of ./bootstrap.sh, that might help future people that try the stunt of not using source packages.

Hence I'd conclude it's not worth the effort. FYI, @r-ryantm also updates packages based on information from repology - meaning that it might be possible for you to only worry about updating sile for your distro alone, and it will then pick up the update based on information from there.

That's fine too, I'm happy with that flow if it works. I update Arch Linux AUR packages usually within minutes, repology picks them up by the next day.

Great. I tried building 0.10.7 with automake 1.15 and interestingly it failed with:

WARNING: 'automake-1.15' is probably too old.
         You should only need it if you modified 'Makefile.am' or
         'configure.ac' or m4 files included by 'configure.ac'.
         The 'automake' program is part of the GNU Automake package:

This is still baffling to me. I'm testing with automake-1.16 locally and don't have this issue.

Ideally, release tarball should not need automake & autoconf at all, but it's not big deal that you do.

As far as I know, we already don't need automake to build the release tarballs, they come with the stuff automake puts in make dist to enable building without itself! You only need automake for git clone or archive snapshot builds.

If there is something I'm doing wrong I'd be happy to fix it, I just don't see it yet.

Feel free to decide what ever you want for 0.10.8 and to ping me to test it.

I just pushed the bootstrap hints I mentioned and fixes for a several problems BSD builds were running into. If you get a chance to try building from master soon before I tag 0.10.8 then great, if not no big deal.

P.S. The next release will have automake 1.16 artifacts because Github‌ Actions have Focal images now so I updated from Bionic.

[alerque]

I've bumped this to v0.10.9 (release notes). Nothing upstream really changes the build since v0.10.8. The major change is that Lua 5.4 is now supported. When the Nix supplied Lua and assorted LuaRocks are updated to 5.4 versions, building SILE against them shouldn't be any trouble.

@doronbehar I think the automake version issues are fixed. It turns out we were bumping a timestamp that made the automake generated scripts that are in the dist package believe they needed to rebuild themselves. That shouldn't be happening hence it shouldn't even be checking versions (and automake isn't needed to build).

[doronbehar]

@doronbehar I think the automake version issues are fixed. It turns out we were bumping a timestamp that made the automake generated scripts that are in the dist package believe they needed to rebuild themselves. That shouldn't be happening hence it shouldn't even be checking versions (and automake isn't needed to build).

Indeed. @alerque I've put all of our changes here in a more organized way (more commits and fixed some other issues) in a branch of mine. I hope you won't mind, but I'm opening a new PR with my version of the changes. Admittedly, our committers (me not included :)) most probably won't stumble upon this PR because it's too further down the flud of PRs we are constantly dealing with :). See further details on my PR at #94168 .

[alerque]

I'm not quite sure why adding more PRs is the solution to drowning in PRs 🤷‍♂️, but I definitely appreciate the help getting this cleaned up and presentable!