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: v0.9.5.1 -> v0.10.3; adjust build process #77738

Merged
merged 7 commits into from Feb 12, 2020

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Jan 15, 2020

Motivation for this change

New release of SILE upstream (announcement) has new dependencies and a different build system. Overall it should be easier to packages, but for this release cycle there are several changes.

Note I am not a Nix user, I am an upstream maintainer trying to facilitate this update. There are several things NOT done yet that I don't know how to fix:

  • The ./configure command needs to be passed the --with-system-luarocks argument if, as in the current package, all the external Lua Rocks are being supplied by the package manager. Alternatively if it would be better for Nix not to bother with those and just have SILE bundle the deps itself, you can just drop all the Lua package deps and not pass this argument and it will bundle everything.
  • The man page should get packaged too, is it?

If there is anything else I can facilitate so this packages easily let me know.

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.

@teto
Copy link
Member

teto commented Jan 15, 2020

Thanks a lot for the effort. I believe it would be best to have a nixos user take over now that you have highlighted the main changes. Some users are using it so we should be able to add maintainers for the package, I remember #56873 for instance. @ck3d would you mind ?

'';

postInstall = ''
install -D -t $out/share/doc/sile documentation/*.pdf
install -D -t $out/share/doc/sile documentation/sile.pdf
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't upstream install the doc ? or provide a target install-doc ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you point me do documentation or a good case study for how make install-doc should work? I'd be happy to get it setup properly upstream but I'm unsure what the outcome should look like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The manual (documentation/sile.pdf) actually comes precompiled in the source tarballs now so you can skip the make documentation unless you are building from git HEAD. Note this is also why Gentium-Book and DeJavu fonts are not requirements. If building the manual you'd need those back.

@@ -46,11 +44,12 @@ stdenv.mkDerivation rec {
enableParallelBuilding = true;

checkPhase = ''
make documentation/developers.pdf documentation/sile.pdf
make documentation examples
Copy link
Member

Choose a reason for hiding this comment

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

weird to build documentation in a checkPhase. Eventually documentation generation could be toggled via a withDoc flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I be adding --with-documentation as a ./configure flag so that make all includes these (or not) without needing to know to trigger the extra targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documentation can be dropped from here, make examples is actually not a bad way to do a simple check that everything is working. The downside is going to be that it downloads a bunch of fonts to do that. make busted would be a better check, but requires extra dependencies. Just checking ./sile --version might be an acceptable check phase.

}:

with stdenv.lib;

let
luaEnv = lua.withPackages(ps: with ps;[ lpeg luaexpat lua-zlib luafilesystem luasocket luasec]);
luaEnv = lua.withPackages(ps: with ps;[cassowary linenoise lpeg lua-zlib lua_cliargs luaepnf luaexpat luafilesystem luarepl luasec luasocket stdlib vstruct]);
Copy link
Member

Choose a reason for hiding this comment

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

I believe some of these packages cassowary/linenoise should be added to nixpkgs since they don't appear in pkgs/development/lua-modules/generated-packages.nix

@ck3d
Copy link
Contributor

ck3d commented Jan 19, 2020

I tried to get the package working, but I struggled with the new lua package dependencies.

Could someone who has experience with lua packages decide which way to go:

  1. package missing lua packages in nixpkgs or
  2. use lua packages provided with sile?

@teto
Copy link
Member

teto commented Jan 19, 2020

We should strive for 1: I can help with packaging the missing lua packages see the (slightly outdated) doc https://github.com/NixOS/nixpkgs/pull/55302/files .
Basically add the name to maintainers/scripts/luarocks-packages.csv and run the associated script to regenenerate pkgs/development/lua-modules/generated-packages.nix (the script should be mentioned at top of that file).

@alerque
Copy link
Contributor Author

alerque commented Jan 24, 2020

v0.10.1 is out upstream. There are no changes that should affect packaging.

@teto teto mentioned this pull request Jan 24, 2020
10 tasks
@teto
Copy link
Member

teto commented Jan 24, 2020

I've packaged the missing lua packages to help with this.

@teto
Copy link
Member

teto commented Jan 25, 2020

@GrahamcOfBorg eval

@marsam
Copy link
Contributor

marsam commented Feb 11, 2020

I managed to build it with 3264166, acf4f8b, and 2ca5d8d

On darwin needs tweaks for #65351

@alerque alerque changed the title sile: v0.9.5.1 -> v0.10.0; adjust build process sile: v0.9.5.1 -> v0.10.3; adjust build process Feb 11, 2020
@ofborg ofborg bot added the 6.topic: lua label Feb 11, 2020
@alerque
Copy link
Contributor Author

alerque commented Feb 11, 2020

@marsam Thanks for your work on this. Please instruct me if I'm doing something wrong handling this PR. I just rebased on master and cherry picked your commits.

Something doesn't seem right about the addition of compat53 in the Lua module list. That module is a collection of shims to add Lua 5.3 features to 5.2 and 5.1. Both the configure process and the runtime should know it is not needed in this case.

@marsam
Copy link
Contributor

marsam commented Feb 11, 2020

Something doesn't seem right about the addition of compat53 in the Lua module list

it's required by configure for lua<5.3, and currently nixpkgs uses lua 5.2, if you don't want compat53 I think you can either patch configure.ac or use lua5_3

https://github.com/sile-typesetter/sile/blob/fba931a416b6f9118a096a170f78929e31a99595/configure.ac#L109-L111

@alerque
Copy link
Contributor Author

alerque commented Feb 11, 2020

@marsam My apologies, I thought Nix was at Lua 5.3. I have removed that commit as it was completely bogus.

@alerque alerque requested a review from ck3d February 12, 2020 12:56
@alerque
Copy link
Contributor Author

alerque commented Feb 12, 2020

I see CI has passed. Dose that mean anything?

From the upstream side the only remaining question I have about this is whether the man page ended up where it was supposed to. Otherwise I think it's up to you Nix folks to review and help check off the regular PR checklist ;-)

Thanks for the help everybody.

@marsam marsam merged commit 802ad5f into NixOS:master Feb 12, 2020
@marsam
Copy link
Contributor

marsam commented Feb 12, 2020

Thank you @alerque, I'm going to address the man page installation in another PR

@alerque
Copy link
Contributor Author

alerque commented Feb 12, 2020

@marsam It seems like make install is putting it in a standard location, so it might already be captured. Thanks again for all the help!

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.

None yet

5 participants