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

sile: 0.10.12 → 0.10.13 #105438

Merged
merged 2 commits into from
Jan 10, 2021
Merged

sile: 0.10.12 → 0.10.13 #105438

merged 2 commits into from
Jan 10, 2021

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Nov 30, 2020

Motivation for this change

Upstream release.

Bumping in the blind here but CI should check this okay ... there are no dependency changes or anything else significant to build & packaging in this release.

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.

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 105438 run on x86_64-linux 1

1 package built:
  • sile

@SuperSandro2000
Copy link
Member

Fails to build on darwin:

cp .libs/justenoughharfbuzz.so .libs/justenoughfontconfig.so  .libs/justenoughlibtexpdf.so .libs/fontmetrics.so .libs/svg.so .libs/justenoughicu.so ../core
make[2]: Leaving directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13/src'
make[2]: Entering directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13'
make[2]: Nothing to be done for 'all-am'.
make[2]: Leaving directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13'
make[1]: Leaving directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13'
@nix { "action": "setPhase", "phase": "checkPhase" }
running tests
check flags: -j4 -l4 SHELL=/nix/store/k89nm2jva0qmvd970f84wq2iq1iwm9bs-bash-4.4-p23/bin/bash VERBOSE=y check
make  all-recursive
make[1]: Entering directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13'
Making all in libtexpdf
make[2]: Entering directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13/libtexpdf'
make  all-am
make[3]: Entering directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13/libtexpdf'
make[3]: Nothing to be done for 'all-am'.
make[3]: Leaving directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13/libtexpdf'
make[2]: Leaving directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13/libtexpdf'
Making all in src
make[2]: Entering directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13/src'
cp .libs/justenoughharfbuzz.so .libs/justenoughfontconfig.so  .libs/justenoughlibtexpdf.so .libs/fontmetrics.so .libs/svg.so .libs/justenoughicu.so ../core
make[2]: Leaving directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13/src'
make[2]: Entering directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13'
make[2]: Nothing to be done for 'all-am'.
make[2]: Leaving directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13'
make[1]: Leaving directory '/private/tmp/nix-build-sile-0.10.13.drv-0/sile-0.10.13'
touch .built-subdirs
output=$(mktemp --suffix=.pdf)
trap 'rm -f $output' EXIT SIGHUP SIGTERM
./sile -o $output - <<< "<sile>foo</sile>"
pdfinfo $output | grep "SILE v0.10.13"
./sile: line 2: SYSTEM_SILE_PATH: command not found
./sile: line 3: SHARED_LIB_EXT: command not found
./sile: line 5: syntax error near unexpected token `('
./sile: line 5: `local executable = debug.getinfo(1, "S").short_src'
Syntax Error: Document stream is empty
make: *** [Makefile:1417: selfcheck] Error 1

@alerque
Copy link
Contributor Author

alerque commented Nov 30, 2020

@SuperSandro2000 I'm not exactly sure what that error is about, but here are a couple things to note:

  1. The error being thrown suggests that automake tooling didn't run properly on sile.in, otherwise those lines would not be placeholders they would be valid Lua code.
  2. This bit of the Makefile.am & configure.ac files hasn't changed at all since the last release, nor indeed many releases before that.
  3. This isn't just a problem with SILE on macOS, the Homebrew builds for this release went off without a hitch and are already in the wild.

I don't know what to say is causing this, that's just some info that might help narrow it down.

@SuperSandro2000
Copy link
Member

Full log https://termbin.com/w11x

I found something interesting.

./libtool: line 8118: func_munge_path_list: command not found

If this does not help just mark it broken on darwin. I have no idea right now either.

@doronbehar
Copy link
Contributor

I think we discussed in the past how we usually handle autoconf / automake (vs Nixpkgs' autoreconfHook) & and whether we usually want / don't want to use source tarballs, or bare git checkouts. This package is sitting in both sides of the fence - it needs the tarball that was generated by the release process of upstream - i.e this tarball but still somehow it requires autoconf and automake during the build, and not autoreconfHook.

I tried getting it to build from a git checkout with this diff:

diff --git i/pkgs/tools/typesetting/sile/default.nix w/pkgs/tools/typesetting/sile/default.nix
index 239bafa05c9..2d22dff650f 100644
--- i/pkgs/tools/typesetting/sile/default.nix
+++ w/pkgs/tools/typesetting/sile/default.nix
@@ -1,10 +1,10 @@
 { stdenv
 , darwin
-, fetchurl
+# , fetchurl
+, fetchFromGitHub
 , makeWrapper
 , pkg-config
-, autoconf
-, automake
+, autoreconfHook
 , poppler_utils
 , harfbuzz
 , icu
@@ -38,21 +38,28 @@ in
 
 stdenv.mkDerivation rec {
   pname = "sile";
-  version = "0.10.12";
+  version = "0.10.13";
 
-  src = fetchurl {
-    url = "https://github.com/sile-typesetter/sile/releases/download/v${version}/${pname}-${version}.tar.xz";
-    sha256 = "0bxm3vhba289vcgpzbs1hz5fjamf0zgxkr7h8vcsiijjjavmv64a";
+  src = fetchFromGitHub {
+    owner = "sile-typesetter";
+    repo = "sile";
+    rev = "v${version}";
+    # url = "https://github.com/sile-typesetter/sile/releases/download/v${version}/${pname}-${version}.tar.xz";
+    sha256 = "pNV2xdFjL3f91rnDLPs1DMr24Mz+ZasOarI+D4q/E+4=";
+    fetchSubmodules = true;
   };
 
+  postUnpack = ''
+    patchShebangs $sourceRoot/build-aux/{decore-automake.sh,list-dist-files.sh,git-version-gen}
+  '';
+
   configureFlags = [
     "--with-system-luarocks"
     "--with-manual"
   ];
 
   nativeBuildInputs = [
-    autoconf
-    automake
+    autoreconfHook
     pkg-config
     makeWrapper
   ];

But due to the build being sandboxed I encountered issues with the fonts that are supposed to be downloaded if the build is done from a git checkout due to:

https://github.com/sile-typesetter/sile/blob/0c9c8bf7a8b0e94d887a4ed055210e32cb471a5c/Makefile-fonts#L35

So @alerque if you could find a way to make it easy for Nix to download all the necessary fonts, that would be great. Otherwise, we can mark it as broken for Darwin.

@WolfangAukang
Copy link
Contributor

Result of nixpkgs-review pr 105438 1

1 package built:
  • sile

@marsam marsam merged commit f7c4e82 into NixOS:master Jan 10, 2021
@@ -109,6 +109,7 @@ stdenv.mkDerivation rec {
'';
homepage = "https://sile-typesetter.org/";
platforms = platforms.unix;
broken = stdenv.isDarwin; # https://github.com/NixOS/nixpkgs/issues/23018
Copy link
Contributor

Choose a reason for hiding this comment

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

@marsam how is this related to the build errors we got on darwin?

Copy link
Contributor

Choose a reason for hiding this comment

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

The error happens during the checkPhase because it tries to call the sile executable, which shebang points to the luaEnv wrapper, hence it refers to that issue.

@alerque
Copy link
Contributor Author

alerque commented Jan 27, 2021

So @alerque if you could find a way to make it easy for Nix to download all the necessary fonts, that would be great. Otherwise, we can mark it as broken for Darwin.

Thanks for the follow up on this @doronbehar. Sorry I've been AWOL‌ for a while. I got a bit buried by life and FOSS stuff got really patchy.

Just to clarify here, building from Git requires font downloads because the manual is not prebuilt in the Git repository (it would be a huge binary blob that updates on every commit). There already is a helper in the Makefile for downloading fonts, but you don't need it. The source tarballs have a prebuilt PDF manual in the package and don't need to download any fonts.

What was really wrong here is that autoconf/automake should not be required by builds from the tarball. I don't quite understand what the resolution was here, but if there is still something in the source tarballs that requires autoconf I'd like to fix it.

@alerque alerque deleted the sile-0.10.13 branch January 27, 2021 12:53
@doronbehar
Copy link
Contributor

What was really wrong here is that autoconf/automake should not be required by builds from the tarball.

Could it be it's needed for the tests? The fact that we need autoconf & automake (for whatever rightful or not reasons), it's not the reason it was broken on Darwin.

I don't quite understand what the resolution was here

See #105438 (review) - it's totally a Nix issue on Darwin. It's possible that prior to this update, sile was broken on darwin, but we didn't know it.

@alerque
Copy link
Contributor Author

alerque commented Jan 28, 2021

Could it be it's needed for the tests?

Yes, that's possible.

The fact that we need autoconf & automake (for whatever rightful or not reasons), it's not the reason it was broken on Darwin.

I'd like to get to the bottom of this and eliminate whatever reasons there may be.

Lets say I had a working Docker or VirtualBox image spun up with Nix on board (I have both). What would be the most expeditious way to play with this formula such that I can edit it to experiment without waiting for CI and such that I can feed it test tarballs of unreleased sources. For science.

@doronbehar
Copy link
Contributor

The issue due to it this is broken on Darwin is very old: #23018 and only a few weeks ago at NixOS/rfcs#75 (comment) we sort of only slightly tackled a close issue related to it.

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.

5 participants