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

bumblebee-status: init at 2.2.0 #254772

Merged
merged 2 commits into from
Nov 26, 2023
Merged

Conversation

augustebaum
Copy link
Contributor

@augustebaum augustebaum commented Sep 12, 2023

Description of changes

This creates a package for bumblebee-status, a status program for use with the i3 window manager (or similar).

Because the bumblebee-status includes a large number of plugins within its source code (see here for the docs and here for the source code), each with its own set of dependencies, the package can be overriden in the form of the argument withPlugins, which is the list of plugins to get the dependencies for. By default the list is empty (i.e. no extra dependencies are installed, although the program still has access to the plugins).

Hence this PR includes two packages: bumblebee-status, and bumblebee-status-full for which all plugin dependencies are added as propagatedBuildInputs.

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.11 Release Notes (or backporting 23.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.

@augustebaum
Copy link
Contributor Author

Some things to note:

  • Since the plugins are all contained within the bumblebee-status repo, the withPlugins override doesn't actually install plugins but rather their propagatedBuildInputs (i.e., kind of, runtime dependencies);
  • Not all plugins have their dependencies packaged with Nix so I've implemented some logic for marking when a dependency is missing; however, I haven't figured out how to actually print warnings.

@augustebaum
Copy link
Contributor Author

@imincik @lorenzleutgeb @andres-nav for your consideration.

@andres-nav
Copy link

andres-nav commented Sep 12, 2023

First of all, you need to separate the changes in two commits. One adding you as a maintiner, and the other one adding the package, see #233506. Also it is better to get the source code from Github in my opinion. For the tests, it is better to disable the tests that don't work, not all of them. And it should be alphabetically ordered in all-packages.

Copy link
Member

@lorenzleutgeb lorenzleutgeb left a comment

Choose a reason for hiding this comment

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

Very clean job, nice!

One thing I don't understand is why you chose to have a list of plugins. Both the filtering and generation of the list of names of plugins are a little unwieldy. If plugins.nix would expose an attribute set, consuming them would be selectedPlugins = lib.filterAttrs (name: _: builtins.elem name withPlugins) allPlugins, a list of plugins names would be allPluginNames = builtins.attrNames allPlugins and you might even be able to get rid of all the name = "..." noise in plugins.nix. In case you have a good reason (which apparently I do not comprehend), just ignore this comment.

@augustebaum
Copy link
Contributor Author

augustebaum commented Sep 13, 2023

One thing I don't understand is why you chose to have a list of plugins

@lorenzleutgeb It used to be an attribute set, but I need the name for the warning messages... which are currently unused right now. Will think about this some more.

@augustebaum
Copy link
Contributor Author

augustebaum commented Sep 13, 2023

Update:

  • Added tests (which required switching to fetchFromGithub)
  • Adding myself to the Nixpkgs maintainers list is now a separate commit
  • Implemented warnings about non-existent plugin dependencies

@augustebaum augustebaum marked this pull request as ready for review September 13, 2023 12:59
@augustebaum
Copy link
Contributor Author

Hopefully final update: in the end I'd rather not package plugins with missing dependencies at all. The plugins in question are not all relevant for NixOS in any case (e.g. the ones that check the status of some other Linux package manager), but hopefully the remaining ones can be packaged at some point.

Copy link
Contributor

@imincik imincik left a comment

Choose a reason for hiding this comment

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

Good work @augustebaum

augustebaum added a commit to augustebaum/home-manager that referenced this pull request Sep 18, 2023
FIXME: This commit enables tests by by adding the `bumblebee-status`
derivation that has been submitted to `nixpkgs` (<NixOS/nixpkgs#254772>).
Hence this commit should be pruned at least when `bumblebee-status` is merged into `nixpkgs-unstable`.
augustebaum added a commit to augustebaum/home-manager that referenced this pull request Sep 18, 2023
FIXME: This commit enables tests by by adding the `bumblebee-status`
derivation that has been submitted to `nixpkgs` (<NixOS/nixpkgs#254772>).
Hence this commit should be pruned at least when `bumblebee-status` is merged into `nixpkgs-unstable`.
augustebaum added a commit to augustebaum/home-manager that referenced this pull request Sep 18, 2023
FIXME: This commit enables tests by by adding the `bumblebee-status`
derivation that has been submitted to `nixpkgs` (<NixOS/nixpkgs#254772>).
Hence this commit should be pruned at least when `bumblebee-status` is merged into `nixpkgs-unstable`.
@augustebaum
Copy link
Contributor Author

Remove redundant bumblebee-status-full package variant.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2697

Copy link
Member

@lorenzleutgeb lorenzleutgeb left a comment

Choose a reason for hiding this comment

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

Only minor comments. Still very clean, and IMO good to merge!

@infinisil
Copy link
Member

We sat together at #ZurichZHF, I have some suggestions against this branch: augustebaum#1

@augustebaum augustebaum changed the title bumblebee-status: init at 2.1.6 bumblebee-status: init at 2.2.0 Nov 26, 2023
@augustebaum augustebaum force-pushed the add-bumblebee-status branch 2 times, most recently from df9f7e2 to d10c9db Compare November 26, 2023 13:59
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Went through some iterations with @augustebaum at #ZurichZHF, looking great now!

@infinisil infinisil merged commit d30a74b into NixOS:master Nov 26, 2023
5 of 6 checks passed
@augustebaum
Copy link
Contributor Author

Thanks for the thoughtful review!

@imincik
Copy link
Contributor

imincik commented Nov 27, 2023

Congratulations @augustebaum !

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

Successfully merging this pull request may close these issues.

None yet

8 participants