-
-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
tracy: fix wayland - remove legacy build, add deps #315205
Conversation
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.
Looks good but if some x11 users see this pls give some feedback
It will break the app for X11 users since the support for it has been put behind the flag since version I'm fine with it now, but, uhm... yeah... Also, see wolfpld/tracy#505. Updating to v5 of |
If X11 requires different deps, how about we split the package between tracy and tracy-x11? I'm sure there's more than zero happy X11 tracy users out there and I don't want to fight them over the steering wheel on who gets to use this program. |
Yep, I also think this is the way to approach this. I would make it somewhat similar to how it's done in the derivation for
|
I have implemented what you described, and tested with xeyes that tracy-x11 is an X program and tracy is a wayland program |
No idea what that test failure is supposed to be |
c9981af
to
b0fd082
Compare
pkgs/by-name/tr/tracy/package.nix
Outdated
, freetype | ||
, glfw | ||
, hicolor-icon-theme | ||
, libxkbcommon | ||
, pkg-config | ||
, tbb | ||
, wayland | ||
, withWayland ? true |
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.
Add a separate block of build inputs, like this:
, capstone
, darwin
, dbus
, freetype
, glfw
, hicolor-icon-theme
, pkg-config
, tbb
, withWayland ? stdenv.isLinux
, libxkbcommon
, wayland
Note that we should also stick to pulling in Wayland dependencies on Linux systems only.
@@ -7584,7 +7584,7 @@ with pkgs; | |||
|
|||
tracker = callPackage ../development/libraries/tracker { }; | |||
|
|||
tracy = callPackage ../development/tools/tracy { }; | |||
tracy-x11 = callPackage ../by-name/tr/tracy/package.nix { withWayland = false; }; |
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.
The LEGACY
flag is about choosing between GLFW and Wayland backends on Linux systems. Meaning, as of right now, the legacy build works both on X11 and Wayland (or should I say, used to work before #308220), so the use of the flag is not really an issue of X11 vs. Wayland. In the mean time, GLFW is still the default on all other platforms (including Darwin, which we support).
I agreed with the creation of the x11
package before, but now I think that we probably shouldn't add the legacy build to the binary cache. This also leads us to having two identical packages on Darwin systems (since the LEGACY
flag is ignored there). So, this line should be deleted, and the release note should mention the use of the flag instead.
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.
I agreed with the creation of the x11 package before, but now I think that we probably shouldn't add the legacy build to the binary cache
I think it's valid to have both in there. We have both X11 and Wayland users, and shouldn't make it unnecessarily painful for X11 users. We build so much other less useful stuff, I think it's fine to build both flavours.
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.
As I described above, this is more of a GLFW vs. custom Wayland backend issue, rather than an X11 vs. Wayland issue. On Darwin, two identical packages will be cached with the current set of changes, and the -x11
postfix doesn't really mean anything on that platform.
I wouldn't say it's painful to override one attribute if the legacy build is needed, but... If @widlarizer still wants to create a separate package, then he needs to make conditional changes to pname
(different name is required to cache a different derivation) and meta.platforms
.
Theoretically, we would have the tracy-wayland
package for Linux only and tracy-glfw
for both Linux and Darwin (the latter is the way it is packaged now). Then, one of those would conditionally be chosen as tracy
.
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.
I'm not saying your assessment is wrong, I'm merely proposing to have that being dealt with in a followup (or as an additional commit to this PR).
The PR fixes a crash at runtime, and already did a lot of rounds. The changes requested again might not be respectful to the PR author, who was merely sending a fix for a crash while being "very sleepy rn and don't really know what [...] doing".
We can always juggle things around a bit in unstable, especially editing release notes or removing "redundant packages".
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.
I like the proposed solution (the one starting with "Theoretically,"). Being an X11 user on Linux is still valid and on NixOS shouldn't require overrides. Only problem is, looking at all-packages.nix for inspiration, I don't see a way of having one or two flavors of a package depending on the system. I'll leave that bit to a potential followup by @paveloom
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.
Tested this on Wayland, and the tracy-x11
attribute still has the same store path as before - so there's not really any way this could have regressed X11 users (other than having to switch to the other attribute, which is clearly documented). I don't think we need to keep this broken for longer ;-)
@widlarizer can you address the cosmetical changes, or should this be done in a followup PR?
b0fd082
to
aefaf8f
Compare
Cosmetics addressed, feel free to merge if CI goes green |
can we backport this to 24.05? |
I don't think so, at least not replacing the |
Description of changes
This fixes #308220 on my end, but consider this a draft PR. I am very sleepy rn and don't really know what I'm doing. Tagging @paveloom @mpickering
Tasks before merging
Things done
I remove
LEGACY=1
. I add wayland, libxkbcommon as deps and replace capstone with capstone_4 since the current default.nix explicitly references capstone 4.nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)