libxkbcommon: build with wayland support tools

[peterhoeg]

Motivation for this change

As wayland is the future anyway, it really doesn't make sense to not have the wayland support tools built with libxkbcommon.

Size difference:
/nix/store/czvbssspmfmm40syq0ylbczf9c4lv6ss-libxkbcommon-1.0.3 45346264
/nix/store/g81gf06jmyxp0j4ymjnjlggz71pz9zcq-libxkbcommon-1.0.3 46533584

Very large rebuild, so staging first.

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]

As wayland is the future anyway, it really doesn't make sense to not have the wayland support tools built with libxkbcommon.

Could you go a bit more into why this is required? See #110693 (comment). The withWaylandSupport "flag" might have a bad name, this is only required for that debugging subcommmand. I'm already happily using libxkbcommon on Wayland.

[primeos]

Why did you change that part? This wasn't an accident, wayland must be in both nativeBuildInputs (wayland-scanner) and buildInputs (libs).

[peterhoeg]

Everything builds fine here without nativeBuildInputs - (with and without the withWaylandSupport flag). How would you reproduce the problem?

[peterhoeg]

Reason for asking - I"ll add in a comment here to explain the situation and avoid the next person removing it if it is truly needed.

[primeos]

Everything builds fine here without nativeBuildInputs

That's an unfortunate issue for backward compatibility / legacy reasons (buildInputs will be added to PATH but that shouldn't be the case). See also https://nixos.org/manual/nixpkgs/unstable/#variables-specifying-dependencies, etc.

How would you reproduce the problem?

E.g. by building pkgsCross.aarch64-multiplatform.libxkbcommon.

Reason for asking - I"ll add in a comment here to explain the situation and avoid the next person removing it if it is truly needed.

It shouldn't require a comment, this is the case for most(?) packages that depend on Wayland (plus a lot of other packages).

[peterhoeg]

Could you go a bit more into why this is required?

As mentioned in the first post, it's just to get the wayland tools.

[primeos]

As mentioned in the first post, it's just to get the wayland tools.

I've read your comments but tbh I'm still not convinced (it's only one tool, or rather a subcommand, and it's an interactive "debugger"). I don't mind but I think we should at least state that in the commit message (to avoid someone reverting this due to concerns that this wasn't really worth it).

[peterhoeg]

@primeos, do you still have concerns with the updated comment?

[primeos]

"This is the wayland equivalent of xev on X11." <- Is there a source for this? I haven't compared the two but IIRC interactive-wayland will/should at least only print keyboard related events. https://git.sr.ht/~sircmpwn/wev (also in Nixpkgs) should be the Wayland equivalent of xev.

[peterhoeg]

"This is the wayland equivalent of xev on X11." <- That comment? Is there a source for this? I haven't compared the two but IIRC interactive-wayland will at least only print keyboard related events.

Sure, right, but do we *really* want to make such a big deal out of what is a tiny issue? The point is, with this flag, you get a sub command that is helpful to some. For everybody else, the price is 1MB. Don't we have bigger things to worry about?

[primeos]

@peterhoeg not sure if my comments came of wrong but I never intended to prevent this from being merged (but I realize now that I forgot to dismiss my earlier review, which was a blocker for me but is resolved now). My intention was simply to raise some concerns to see if they where considered so that we don't regret the decision later (and personally I don't see this change as really necessary but that doesn't mean that I'm against it - feel free to merge it). The very slightly increased closure size is no problem at all IMO but the addition of Wayland into the dependency chain could theoretically cause some issues (but if that would happen we could still look for other solutions than reverting the change).

Anyway, the tl;dr is to regard my position as neutral and that I have no issue if you or anyone else merges this PR. Sorry if that came of wrong.

Oops, I wanted to dismiss my review / be neutral instead of approving

[peterhoeg]

but the addition of Wayland into the dependency chain could theoretically cause some issues

Such as?

Anyway, the tl;dr is to regard my position as neutral and that I have no issue if you or anyone else merges this PR. Sorry if that came of wrong.

Nothing to apologize for - all good! I hope my earlier message didn't come across as confrontational. Just trying to move this forward.

[primeos]

Such as?

Should hopefully be very unlikely but e.g. dependency cycles (wayland's dependencies for documentation aren't low-level) and it could (but shouldn't) make cross-compilation more difficult. But that's all speculation (I haven't looked at the dependency graph) and shouldn't matter for now.

Normally I wouldn't even bring it up but in this case I made an exception because libxkbcommon is used as a library and I feel like few to none users have libxkbcommon.bin installed which is why I don't see a practical advantage (so it isn't that I'm particularly concerned about potential drawbacks, it's just that I don't see a useful (enough) advantage / that we're enabling something simply because we can without really knowing if it'll be helpful/useful - but that's just a very subjective opinion).
(And I'm not that happy with some irrelevant details like the reformatting, the renaming of withWaylandSupport (both names aren't ideal and it requires changes in case anyone already uses it), and the new "This is the wayland equivalent of xev on X11." comment - but that's all irrelevant/nitpicking/bikeshedding)

I hope my earlier message didn't come across as confrontational. Just trying to
move this forward.

No problem, it didn't come across as confrontational. I just wanted to say that I'm not trying to prevent this from moving forward.

Tbh I start regretting to even bring up those concerns as this has lead to unnecessary bikeshedding over a simple change that should even be very easy to revert at any point in the future in case we'll run into any (minor) issues that are difficult to avoid.
So to make this easier I'll just approve it so that we can move this forward :D (but I'm basically neutral here as I don't see any real reasons for/against this change)

Requested changes where resolved

[SuperSandro2000]

diff LGTM