-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
wlroots: 0.14.1 -> 0.15.0 #151234
wlroots: 0.14.1 -> 0.15.0 #151234
Conversation
e7619c7
to
ebe75e8
Compare
Supersedes #144142
…On Sat, 18 Dec 2021, 23:48 ofborg[bot], ***@***.***> wrote:
@ofborg <https://github.com/ofborg>[bot] requested your review on: #151234
<#151234> wlroots: 0.14.1 -> 0.15.0.
—
Reply to this email directly, view it on GitHub
<#151234 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABV7PJ5U4UG5MAZVAC2O4TLURUFSVANCNFSM5KLCXZGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
@primeos I pushed some work I had locally on this same bump onto your branch, feel free to drop my commits though if you think they increase complexity too much. I just made the renderer and backends configurable as derivation args. |
@lovesegfault thanks for your efforts. Unfortunately I do disagree with most of those changes though. Could you provide a rationale for them (i.e. is there also a use-case in or outside Nixpkgs that requires this degree of customization)? I'd just keep enabling all features for now and only introduce such options when there's a need for them (to keep unnecessary complexity to a minimum). I'm also against setting options to their defaults ( This is just my personal opinion though and it's up for debate ;) |
There are new things using Mostly, I just see
That's fair enough, as I said I'm absolutely fine with dropping my commits, I just had them ready and wanted them to be outside of my local checkout in case there was interest. We could just revisit this when someone opens an issue for this.
I feel mixed about this, IMHO if we have code to install examples (which we do) we should be manually saying "please build examples" to prevent this from silently breaking if examples ever default to false. Likewise, if we're going through the trouble of bringing in xcb-errors as a dependency (which we are) we should also be making sure it's being used by enabling it. I mostly fear a scenario where their build system decides it will look for xcb-errors differently, won't find ours, and the build will start to happen without xcb-errors silently (as opposed to erroring if the feature was manually enabled).
I think we should revert/drop my commits for now, once this lands I'm happy to place them in a new/separate PR and further discussion can happen there. I probably should have done that to begin with :) |
Ok, your points are also valid. If I think about it I'd prefer to keep this out of this PR though as this is only about updating wlroots and should try to cause as little discussion as possible (it should be a simple bump but of course there are already a few additional design decisions that have to be made). Feel free to open a dedicated PR if you want ;)
The examples are optional and we could rewrite
No, that's my point, unless upstream changes the default or we drop
+1, thanks :) We can hopefully merge this PR soon. Regarding this PR: I'm currently waiting a bit until the first reverse-dependencies start to support version 0.15 but we could already merge it (as |
Maybe merge, but set wlroots = wlroots_0_14 until Sway updates?
…On Tue, 21 Dec 2021, 21:12 Michael Weiss, ***@***.***> wrote:
That's fair enough, as I said I'm absolutely fine with dropping my
commits, I just had them ready and wanted them to be outside of my local
checkout in case there was interest. We could just revisit this when
someone opens an issue for this.
Ok, your points are also valid. If I think about it I'd prefer to keep
this out of this PR though as this is only about updating wlroots and
should try to cause as little discussion as possible (it should be a simple
bump but of course there are already a few additional design decisions that
have to be made). Feel free to open a dedicated PR if you want ;)
I feel mixed about this, IMHO if we have code to install examples (which
we do) we should be manually saying "please build examples" to prevent this
from silently breaking if examples ever default to false.
The examples are optional and we could rewrite postFixup to detect this.
It's also unlikely that they silently change the default and I always keep
an eye on such changes.
I mostly fear a scenario where their build system decides it will look for
xcb-errors differently, won't find ours, and the build will start to happen
without xcb-errors silently (as opposed to erroring if the feature was
manually enabled).
No, that's my point, unless upstream changes the default or we drop
-Dauto_features=enabled it doesn't make a difference. And after all we
have -Dauto_features=enabled for a reason.
I think we should revert/drop my commits for now, once this lands I'm
happy to place them in a new/separate PR and further discussion can happen
there. I probably should have done that to begin with :)
+1, thanks :) We can hopefully merge this PR soon.
Regarding this PR: I'm currently waiting a bit until the first
reverse-dependencies start to support version 0.15 but we could already
merge it regardless, if required.
—
Reply to this email directly, view it on GitHub
<#151234 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABV7PJ3V4TQRMUVRWXCZVCTUSDNTRANCNFSM5KLCXZGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
@Synthetica9 ok, yeah, I was thinking of setting nixpkgs-review result:
Should be good to go then :) PS: I split the changes into two commits to make it easier to review/track via the Git history (otherwise Git should determine that default.nix was renamed to 0.14.nix and it'll be more difficult to see the changes from 0.14 -> 0.15). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the split commits :)
(I think we should take an example from other packages that expect to have multiple versions and set |
@Synthetica9 ok, I misunderstood your previous comment then, so should I change it to this?: wlroots = wlroots_0_15;
wlroots_0_12 = callPackage ../development/libraries/wlroots/0.12.nix {};
wlroots_0_14 = callPackage ../development/libraries/wlroots/0.14.nix {
inherit (xorg) xcbutilrenderutil;
};
wlroots_0_15 = callPackage ../development/libraries/wlroots/0.15.nix {
inherit (xorg) xcbutilrenderutil;
}; |
Exactly! |
0.14.nix is an exact copy of default.nix. This no-op change is made in preparation of introducing wlroots 0.15.0 which would break most reverse-dependencies.
Release notes: https://gitlab.freedesktop.org/wlroots/wlroots/-/tags/0.15.0 Only three reverse-dependencies are compatible with the new release so far.
Done:
|
Ok, let's merge it then 🎉. I've already opened #151311 to track the status of all reverse dependencies. |
Release notes: https://gitlab.freedesktop.org/wlroots/wlroots/-/tags/0.15.0
https://gitlab.freedesktop.org/wlroots/wlroots/-/tags/0.15.0#changes-for-packagers
Motivation for this change
Note: Needs to target
staging
because #148588 isn't even instaging-next
yet.It might also be possible to avoid overriding Meson but reverting https://gitlab.freedesktop.org/wlroots/wlroots/-/commit/31db23270426e47ca6487997ef3af25474593802 alone isn't sufficient (I might revisit this later but overriding Meson might be the better solution for now). Update: It looks like #144779 might make it in time :)I missed #150486. There are quite a few PRs regarding Meson... :oAnyway, this is only a preliminary draft as most packages don't support wlroots 0.15.0 yet. We're in no hurry as this depends on staging so we can wait for a few reverse-dependencies to release updates (and for the rest we might have to keep wlroots 0.14 around).
nixpkgs-review
output
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes