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.

Sign up for GitHub

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

Jump to bottom

libxkbcommon: Enable libxkbregistry #110773

Merged
merged 1 commit into from Jan 25, 2021
Merged

libxkbcommon: Enable libxkbregistry #110773

merged 1 commit into from Jan 25, 2021

Conversation

primeos
Copy link
Member

@primeos primeos commented Jan 25, 2021

This also enables the "list" subcommand of xkbcli.

Since libxkbregistry is an optional library we could install it into a
different output. However, doing this properly is quite challenging and
the best approach would likely be to upstream patches that add a Meson
option for installing libxkbregistry under a separate prefix (so that
the pkg-config file is generated correctly, etc.).
But even then the default fixup phase would try to move
$libxkbregistry/include into the $dev output and the $out output would
depend on the $libxkbcommon output because of the xkbcli binary (though
we could move that into a $bin output).
As a result it seems best not to install libxkbregistry into a dedicated
output path.

Motivation for this change
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.

@primeos
libxkbcommon: Enable libxkbregistry
3762bb5 
This also enables the "list" subcommand of xkbcli.

Since libxkbregistry is an optional library we could install it into a
different output. However, doing this properly is quite challenging and
the best approach would likely be to upstream patches that add a Meson
option for installing libxkbregistry under a separate prefix (so that
the pkg-config file is generated correctly, etc.).
But even then the default fixup phase would try to move
$libxkbregistry/include into the $dev output and the $out output would
depend on the $libxkbcommon output because of the xkbcli binary (though
we could move that into a $bin output).
As a result it seems best not to install libxkbregistry into a dedicated
output path.
@primeos primeos mentioned this pull request Jan 25, 2021
Merged
10 tasks
@primeos
Copy link
Member Author

primeos commented Jan 25, 2021 •
edited

This is a followup on #108004. We could e.g. do one of the following things:

  • Merge this PR
  • Ask upstream on their opinion how this should be packaged (as part of libxkbcommon or as a separate package)
  • Close this PR and keep libxkbregistry unpackaged for now (until some new packages will depend on it)
    • But in this case we should probably add some notes to pkgs/development/libraries/libxkbcommon/default.nix for the next discussion

Some data regarding other Linux distributions:

@ofborg ofborg bot requested a review from ttuegel January 25, 2021 14:12
@SuperSandro2000
Copy link
Member

Debian packages it as libxkbcommon-doc, libxkbregistry-dev and libxkbregistry0.

But please remember that Debian creates a package for almost anything.

@jtojnar
Copy link
Contributor

jtojnar commented Jan 25, 2021 •
edited

@primeos Do you know how much it adds in terms of closure size?

Debian packages it as libxkbcommon-doc, libxkbregistry-dev and libxkbregistry0.

Debian benefits from the fact that it uses FHS so, for them, splitting a package is just moving files around. For us, it requires mucking around with the project’s build system.

I want to make it easier for Meson projects to create subprojects that can be installed separately but did not get to it yet.

@primeos
Copy link
Member Author

primeos commented Jan 25, 2021

@primeos Do you know how much it adds in terms of closure size?

Right, I forgot to check and provide that information :o

Console output:

$ # Before / without this PR:
$ for output in out dev doc; do nix path-info --size $(nix-build -A libxkbcommon.$output) && nix path-info --closure-size $(nix-build -A libxkbcommon.$output); done | column -t
/nix/store/ks6j3z7ycp9y246dw09pbhgrsci45a5j-libxkbcommon-1.0.3      449536
/nix/store/ks6j3z7ycp9y246dw09pbhgrsci45a5j-libxkbcommon-1.0.3      43429712
/nix/store/ckirmdja00wb209aqqlgwz9f3j0i1m11-libxkbcommon-1.0.3-dev  314272
/nix/store/ckirmdja00wb209aqqlgwz9f3j0i1m11-libxkbcommon-1.0.3-dev  43743984
/nix/store/h8gkpsx3bkl57j9mclmmiqvyh852lkfk-libxkbcommon-1.0.3-doc  1026736
/nix/store/h8gkpsx3bkl57j9mclmmiqvyh852lkfk-libxkbcommon-1.0.3-doc  1026736
$ # After / with this PR:
$ for output in out dev doc; do nix path-info --size $(nix-build -A libxkbcommon.$output) && nix path-info --closure-size $(nix-build -A libxkbcommon.$output); done | column -t
/nix/store/0ahpqm5ll42bbrkiw052gll8wv67mswh-libxkbcommon-1.0.3      512160
/nix/store/0ahpqm5ll42bbrkiw052gll8wv67mswh-libxkbcommon-1.0.3      45346264
/nix/store/fc0xcnyhj2amk47hjq7zaqgz6ngvpgiz-libxkbcommon-1.0.3-dev  339136
/nix/store/fc0xcnyhj2amk47hjq7zaqgz6ngvpgiz-libxkbcommon-1.0.3-dev  45685400
/nix/store/cq7ws1w9sc1ygi4zr0zlg23mc5admrw1-libxkbcommon-1.0.3-doc  1026736
/nix/store/cq7ws1w9sc1ygi4zr0zlg23mc5admrw1-libxkbcommon-1.0.3-doc  1026736

I forgot to mention that the libxml2 dependency which I added in the previous PR is only needed for libxkbregistry (I only realized that later while looking at meson.build). Luckily it isn't that big though:

+---/nix/store/4cny85dmpnfcfvmf7s50q90gn8pngna2-libxml2-2.9.10
|   +---/nix/store/gafigwfaimlziam6qhw1m8dz4h952g1n-glibc-2.32-35 [...]
|   +---/nix/store/lnjmyxgqq27dsmnc2sxwa7hn448bzc4k-zlib-1.2.11
|   |   +---/nix/store/gafigwfaimlziam6qhw1m8dz4h952g1n-glibc-2.32-35 [...]
|   +---/nix/store/4cny85dmpnfcfvmf7s50q90gn8pngna2-libxml2-2.9.10 [...]

The main contributor to the libxml2 closure is glibc but that's obviously already in libxkbcommon's closure anyway:

$ nix path-info -S /nix/store/4cny85dmpnfcfvmf7s50q90gn8pngna2-libxml2-2.9.10
/nix/store/4cny85dmpnfcfvmf7s50q90gn8pngna2-libxml2-2.9.10	   34791952
$ nix path-info -S /nix/store/gafigwfaimlziam6qhw1m8dz4h952g1n-glibc-2.32-35
/nix/store/gafigwfaimlziam6qhw1m8dz4h952g1n-glibc-2.32-35	   32938024

So I assume it should be fine.

jtojnar
jtojnar approved these changes Jan 25, 2021
View reviewed changes
Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

LGTM then.

@SuperSandro2000 SuperSandro2000 merged commit 90cca37 into NixOS:staging Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar approved these changes

@ttuegel ttuegel Awaiting requested review from ttuegel

Assignees
No one assigned
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 1001-2500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@primeos @SuperSandro2000 @jtojnar