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
Merged

Conversation

@alerque
Copy link
Contributor

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.
@alerque alerque mentioned this pull request Jan 15, 2020
18 of 21 tasks complete
@alerque alerque force-pushed the alerque:sile-v0.10.0 branch from 7aa9526 to 3f7d02c Jan 15, 2020
@teto

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
@teto

teto Jan 15, 2020

Contributor

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

This comment has been minimized.

Copy link
@alerque

alerque Jan 24, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@alerque

alerque Jan 24, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@teto

teto Jan 15, 2020

Contributor

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

This comment has been minimized.

Copy link
@alerque

alerque Jan 24, 2020

Author Contributor

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?

This comment has been minimized.

Copy link
@alerque

alerque Jan 24, 2020

Author Contributor

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]);

This comment has been minimized.

Copy link
@teto

teto Jan 15, 2020

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

alerque commented Jan 24, 2020

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

@alerque alerque mentioned this pull request Jan 24, 2020
17 of 21 tasks complete
@teto teto mentioned this pull request Jan 24, 2020
0 of 10 tasks complete
@teto

This comment has been minimized.

Copy link
Contributor

teto commented Jan 24, 2020

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

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Jan 25, 2020

@marsam

This comment has been minimized.

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
@alerque alerque force-pushed the alerque:sile-v0.10.0 branch from 3deed57 to 4993695 Feb 11, 2020
@ofborg ofborg bot added the 6.topic: lua label Feb 11, 2020
@alerque

This comment has been minimized.

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

This comment has been minimized.

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 and others added 5 commits Jan 15, 2020
alerque and others added 2 commits Feb 5, 2020
@alerque alerque force-pushed the alerque:sile-v0.10.0 branch from 878d08b to 984256e Feb 11, 2020
@alerque

This comment has been minimized.

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 Feb 12, 2020
@alerque

This comment has been minimized.

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
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
luaPackages.cassowary, luaPackages.cosmo, sile on aarch64-linux Success
Details
luaPackages.cassowary, luaPackages.cosmo, sile on x86_64-linux Success
Details
@marsam

This comment has been minimized.

Copy link
Contributor

marsam commented Feb 12, 2020

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

@alerque

This comment has been minimized.

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!

@alerque alerque deleted the alerque:sile-v0.10.0 branch Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.