Skip to content

Revert "wayland: mark as broken on darwin"#214828

Merged
figsoda merged 1 commit intoNixOS:masterfrom
alyssais:wayland-revert
Feb 6, 2023
Merged

Revert "wayland: mark as broken on darwin"#214828
figsoda merged 1 commit intoNixOS:masterfrom
alyssais:wayland-revert

Conversation

@alyssais
Copy link
Member

@alyssais alyssais commented Feb 5, 2023

Description of changes

This reverts commit d9986a5.

That commit turned a package that built for macOS into a package that was marked broken for both (also breaking the wayland-scanner attribute, which is important for cross-compiling to Linux), and complicated the mesa-demos expression, with no explanation given and only four hours of opportunity for review.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@alyssais
Revert "wayland: mark as broken on darwin"
1c561ba 
This reverts commit d9986a5.

That commit turned a package that built for macOS into a package that
was marked broken for both (also breaking the wayland-scanner
attribute, which is important for cross-compiling to Linux), and
complicated the mesa-demos expression, with no explanation given and
only four hours of opportunity for review.
@alyssais alyssais requested review from primeos and wegank February 5, 2023 22:22
@ofborg ofborg bot added 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Feb 5, 2023
@figsoda figsoda merged commit c74f9dd into NixOS:master Feb 6, 2023
@alyssais alyssais deleted the wayland-revert branch February 6, 2023 01:06
@wegank
Copy link
Member

wegank commented Feb 6, 2023 •
edited
Loading

Strong objection. The compiled Wayland package contains only stub files:

$ find result/
result/
result//share
result//share/wayland
result//share/wayland/wayland.dtd
result//share/wayland/wayland.xml
result//share/wayland/wayland-scanner.mk

which means that almost all packages previously marked as broken transitively by Wayland are now throwing an error on Hydra complaining about the absence of wayland-scanner or any libraries, if one ever run an nixpkgs-review on this PR.

So I'm going to post another PR in a few hours to reject this one. You are welcome to disagree with me in that PR.

Besides, this PR left three hours for review, and that's from 11pm to 2am for me.

@wegank wegank mentioned this pull request Feb 6, 2023
Closed
13 tasks
@alyssais
Copy link
Member Author

alyssais commented Feb 6, 2023

Besides, this PR left three hours for review, and that's from 11pm to 2am for me.

This PR was independently reviewed and merged, not an unreviewed self merge.

@wegank
Copy link
Member

wegank commented Feb 6, 2023

Besides, this PR left three hours for review, and that's from 11pm to 2am for me.

This PR was independently reviewed and merged, not an unreviewed self merge.

I admit it's better than what I did, anyway.

@primeos
Copy link
Member

primeos commented Feb 6, 2023

with no explanation given and only four hours of opportunity for review.

Yes. @wegank: please write a proper commit message next time (important for any non-trivial changes) and leave more time for reviews. Thanks :)

almost all packages previously marked as broken transitively by Wayland are now throwing an error on Hydra complaining about the absence of wayland-scanner or any libraries

Not sure if that's true. AFAIK Wayland shouldn't be required on Darwin so those packages might only depend on it due to Nixpkgs dependency inaccuracies. I'm neither using Darwin nor am I interested in Darwin though so I have no idea / didn't look into it.

Besides, this PR left three hours for review, and that's from 11pm to 2am for me.

IMO that's fine for reverting problematic changes / significant regressions (they can always be re-landed / fixed later if necessary). I'm not sure how significant this regression was but I'd probably merged this PR as soon as CI passed.

Anyway, no hard feelings but as maintainer of the Wayland package I just wanted to share my opinion on this matter as well.
And thanks @alyssais for catching this :)

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

Reviewers

@primeos primeos Awaiting requested review from primeos

@wegank wegank Awaiting requested review from wegank

1 more reviewer

@figsoda figsoda figsoda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alyssais @wegank @primeos @figsoda