Skip to content

wayland: don't propagate bin output#328804

Merged
flokli merged 1 commit intoNixOS:stagingfrom
flokli:wayland-no-propagate-bin
Jul 21, 2024
Merged

wayland: don't propagate bin output#328804
flokli merged 1 commit intoNixOS:stagingfrom
flokli:wayland-no-propagate-bin

Conversation

@flokli
Copy link
Member

@flokli flokli commented Jul 21, 2024

This fixes the host platform wayland-scanner binary leaking into cross builds.

Fixes pkgsCross.aarch64-multiplatform.glfw for example.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux (via pkgsCross from x86_64)
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@flokli flokli requested review from K900 and SomeoneSerge July 21, 2024 02:17
SomeoneSerge
SomeoneSerge reviewed Jul 21, 2024
View reviewed changes
@ofborg ofborg bot requested review from alyssais, codyopel and primeos July 21, 2024 02:58
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jul 21, 2024
@alyssais
Copy link
Member

alyssais commented Jul 21, 2024 •
edited by flokli
Loading

TIL propagatedBuildOutputs!

I should really finish #214906 …

SomeoneSerge
SomeoneSerge reviewed Jul 21, 2024
View reviewed changes
pkgs/development/libraries/wayland/default.nix
Copy link
Contributor

@SomeoneSerge SomeoneSerge Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

On a more general note:

  • This should maybe exclude "binaries":

    nixpkgs/pkgs/build-support/setup-hooks/multiple-outputs.sh

    Lines 195 to 197 in db5bef9

    # Default value: propagate binaries, includes and libraries
    if [ -z "${propagatedBuildOutputs+1}" ]; then
    local po_dirty="$outputBin $outputInclude $outputLib"
  • It's often awkward we cannot get headers (which we put into .dev) separately from propagatedBuildOutputs. In this case it was an issue for cross-compilation. Another instance where I encountered this is when I wanted to build scipy against numpy_2 and propagate numpy (<2) for wider compatibility

@SomeoneSerge SomeoneSerge mentioned this pull request Jul 21, 2024
Open
13 tasks
@flokli
wayland: don't propagate bin output
3f15de5 
This fixes the host platform wayland-scanner binary
leaking into cross builds.
@flokli flokli marked this pull request as draft July 21, 2024 13:05
@flokli flokli changed the base branch from master to staging July 21, 2024 13:06
@flokli flokli force-pushed the wayland-no-propagate-bin branch from 2e85626 to 3f15de5 Compare July 21, 2024 13:06
@flokli flokli marked this pull request as ready for review July 21, 2024 13:06
@flokli
Copy link
Member Author

flokli commented Jul 21, 2024

Added the comment, and re-targeted to staging.

@flokli flokli requested a review from SomeoneSerge July 21, 2024 13:11
@flokli flokli merged commit 38b5a20 into NixOS:staging Jul 21, 2024
@flokli flokli deleted the wayland-no-propagate-bin branch July 21, 2024 22:40
@trofi
Copy link
Contributor

trofi commented Jul 23, 2024

Bisect says the change broke gst_all_1.gst-plugins-base in staging as:

$ nix build --no-link -f. -L gst_all_1.gst-plugins-base
...
gst-plugins-base> Program wayland-scanner found: NO
gst-plugins-base> gst-libs/gst/gl/meson.build:674:8: ERROR: Problem encountered: Could not find requested Wayland libraries

@flokli
Copy link
Member Author

flokli commented Jul 23, 2024

Thanks for the ping, will take a look.

@flokli flokli mentioned this pull request Jul 23, 2024
Closed
13 tasks
flokli added a commit to flokli/nixpkgs that referenced this pull request Jul 23, 2024
@flokli
gst_all_1.gst-plugins-base: fix compilation
9c17c51 
`wayland-scanner` is in `wayland.bin`.
This previously only worked as wayland propagated its `bin` output.

See NixOS#328804 for context.
@flokli
Copy link
Member Author

flokli commented Jul 23, 2024

Fix in #329452. Will quickly check all other gst packages.

flokli added a commit to flokli/nixpkgs that referenced this pull request Jul 23, 2024
@flokli
Revert "wayland: don't propagate bin output"
0b29e62 
This reverts commit 3f15de5.

As commented in
NixOS#328804 (comment),
this broke quite some builds, and some of the interactions between
wayland and its `.pc` file are still uncertain. Revert for now, and
re-roll this later.
@flokli flokli mentioned this pull request Jul 23, 2024
Merged
13 tasks
@emilazy
Copy link
Member

emilazy commented Jul 27, 2024

It should be possible to re‐apply this now that @paparodeo’s work in #318226 has landed in staging.

@SomeoneSerge
Copy link
Contributor

SomeoneSerge commented Jul 27, 2024

It should be possible to re‐apply this now

It shouldn't be necessary

@SomeoneSerge
Copy link
Contributor

SomeoneSerge commented Jul 27, 2024 •
edited
Loading

In fact, I think we should maybe remove the custom .pc from $bin, and also maybe even delete wayland-scanner = wayland.bin from the package set. The dev in nativeBuildInputs propagating bin should now work perfectly fine, so it is with wayland-scanner.pc. We'd just write nativeBuildInputs = [ wayland ]; buildInputs = [ wayland ]; to get both the scanner and the libraries

@alyssais
Copy link
Member

alyssais commented Jul 28, 2024

That causes its own problems, see #214895.

But there's no overlap really between wayland and wayland-scanner, so the best thing to do is probably to have it as a completely separate package: #214906

@SomeoneSerge
Copy link
Contributor

SomeoneSerge commented Jul 28, 2024

Thanks @alyssais I didn't know about this one. A separate derivation sounds even better

@alyssais
Copy link
Member

alyssais commented Jul 28, 2024

(If anybody wants to take over that PR please do — the main thing that was hard about it was checking there were no cross regressions, but tbh mostly I think I got distracted by trying to fix packages that didn't cross compile anyway! I don't know when I'll have time to pick it back up now.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SomeoneSerge SomeoneSerge SomeoneSerge approved these changes

@K900 K900 Awaiting requested review from K900

@primeos primeos Awaiting requested review from primeos

@alyssais alyssais Awaiting requested review from alyssais

@codyopel codyopel Awaiting requested review from codyopel

Assignees

No one assigned

Labels

10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@flokli @alyssais @trofi @emilazy @SomeoneSerge