Skip to content
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

haskellPackages.taffybar: fix build #122705

Merged
merged 3 commits into from
May 13, 2021
Merged

haskellPackages.taffybar: fix build #122705

merged 3 commits into from
May 13, 2021

Conversation

rvl
Copy link
Contributor

@rvl rvl commented May 12, 2021

Motivation for this change

Add patches to allow building with gi-gdkpixbuf_2_0_26

Upstream PRs:

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.

@@ -1271,6 +1271,20 @@ self: super: {
# Created upstream PR @ https://github.com/ghcjs/jsaddle/pull/119
jsaddle-webkit2gtk = appendPatch super.jsaddle-webkit2gtk ./patches/jsaddle-webkit2gtk.patch;

# 2021-05-12: gi-gdkpixbuf_2_0_26 fix
# https://github.com/taffybar/gtk-sni-tray/pull/25
gtk-sni-tray = markUnbroken (appendPatch super.gtk-sni-tray (pkgs.fetchpatch {
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use markUnbroken in nixpkgs itself, instead remove the packages in question from pkgs/development/haskell-modules/configuration-hackage2nix/broken.yaml and run ./maintainers/scripts/haskell/regenerate-hackage-packages.sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK thanks, fixed.

@sternenseemann
Copy link
Member

taffybar probably warrants a fix in master, we probably should add that to mergeable (we really need to expand the list tbh).

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 12, 2021
@maralorn
Copy link
Member

taffybar probably warrants a fix in master, we probably should add that to mergeable (we really need to expand the list tbh).

Does it? I knew that taffybar was broken. I even opened an issue. But taffybar does not have a maintainer. So I didn‘t care very much. Before we put stuff in mergeable we should find maintainers for those packages.

@rvl do you by any chance want to be a taffybar maintainer?

Cons: You get pinged when it breaks.
Pros: You get pinged when it breaks. And we know that someone cares about the package.

It’s nothing more than that. You are free to ignore the ping when it breaks.

@sternenseemann
Copy link
Member

sternenseemann commented May 12, 2021

Does it? I knew that taffybar was broken. I even opened an issue. But taffybar does not have a maintainer. So I didn‘t care very much. Before we put stuff in mergeable we should find maintainers for those packages.

IMO that is too careless an approach. People use taffybar as part of their desktop setup and as far as I am aware it is not part of any test which controls the unstable channel. Thus we risk breaking downstream user's machines via channel updates if we adopt such a carefree well-it-doesn't-have-a-maintainer approach.

I'm more comfortable with breaking development tools and library dependencies, but I feel like we should invest extra time in keeping user-facing derivations working. The channel may be called unstable, but that doesn't mean we shouldn't care about breaking stuff.

@maralorn
Copy link
Member

IMO that is too careless an approach. People use taffybar as part of their desktop setup and as far as I am aware it is not part of any test which controls the unstable channel. Thus we risk breaking downstream user's machines via channel updates if we adopt such a carefree well-it-doesn't-have-a-maintainer approach.

Well, I didn‘t know that. That’s my point. I have no clue how many users taffybar has. I have heard about maybe 3. The point about maintained+mergeable jobs is that I don‘t have to make these judgement calls. If taffybar is that essential it should at least have a maintainer.
(I am pretty certain that peti didn‘t watch out for breaking taffybar.)

I'm more comfortable with breaking development tools and library dependencies, but I feel like we should invest extra time in keeping user-facing derivations working. The channel may be called unstable, but that doesn't mean we shouldn't care about breaking stuff.

In general I totally agree.

@rvl
Copy link
Contributor Author

rvl commented May 12, 2021

Yes Taffybar is a desktop component and it has a few users.
There would probably be more users but it's devillishly hard to build and more often broken than not in nixos-unstable.
We got it building for nixos-20.03 and nixos-20.09 though, and it would be nice to have a working Taffybar in nixos-21.05 too.
I added myself to the maintainers list, FWIW, and repaired a change which broke my local config.

@maralorn
Copy link
Member

Yes I remember that it was broken for good parts of the last year. But we can certainly try to increase the quality level here.

Add patches to allow building with gi-gdkpixbuf_2_0_26

Upstream PRs:
 - taffybar/gtk-sni-tray#25
 - taffybar/taffybar#507
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 12, 2021
@sternenseemann
Copy link
Member

@ofborg build taffybar

passthru.env = taffybarEnv;

# Will be faster to build the wrapper locally than to fetch it from a binary cache.
preferLocalBuild = true;
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell preferLocalBuild will not prevent fetching from binary caches.
(There is a lot of uncertainty around this topic, I‘d be happy about a definitive answer.)

Copy link
Member

Choose a reason for hiding this comment

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

allowSubstitutes does that. preferLocalBuild prevents remote builders from being used as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's cargo-culted from other wrappers.
It seems like something users would want, given that the packages parameter is configurable.
But I have no idea whether it's effective.
Perhaps it needs allowSubstitutes = false; too?

Copy link
Member

Choose a reason for hiding this comment

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

We can leave the flag. (It doesn‘t really matter that much either way.) I just believe the comment is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I fixed the comment and also added allowSubstitutes=false because these two flags seem to be commonly used together for trivial derivations which often need to be built locally.

@sternenseemann
Copy link
Member

@ofborg build taffybar

rvl added 2 commits May 13, 2021 10:01
Add myself as a "maintainer" of taffybar, and also arbtt and lentil,
for which I have made build fixes in the past.
@maralorn
Copy link
Member

Thank you for fixing this! And for maintaining stuff!

@maralorn maralorn merged commit 5687a34 into NixOS:master May 13, 2021
@colonelpanic8
Copy link
Contributor

@maralorn @rvl As the primary maintainer of taffybar (who is also a nix and nixos user) I'd prefer it if I was consulted before things like this were done in nixpkgs. I have merged @rvl 's change to gtk-sni-tray, and bumped its version, and made my own fix for taffybar itself, that should allow us to simply use newer versions that are actually on hackage instead of using patches.

@sternenseemann
Copy link
Member

sternenseemann commented May 16, 2021

Feel free to add yourself as a maintainer by adding a corresponding entry to pkgs/development/haskell-modules/configuration-hackage2nix/main.yml. :)

@maralorn
Copy link
Member

Oh, @IvanMalison that is great to know! And yeah, I really think that you should enter yourself as maintainer then.

@rvl
Copy link
Contributor Author

rvl commented May 17, 2021

Hi @IvanMalison, I did link this PR with the gtk-sni-tray and taffybar PRs. We can simply remove those appendPatch overrides next time the Haskell packages set gets updated - whenever that might be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: haskell 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants