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

tmuxifier: init at 0.13.0 #254328

Merged
merged 2 commits into from
Sep 21, 2023
Merged

tmuxifier: init at 0.13.0 #254328

merged 2 commits into from
Sep 21, 2023

Conversation

wigust
Copy link
Contributor

@wigust wigust commented Sep 10, 2023

Description of changes

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.

This pull request will add requested in #226393 package. The Nix package tested on my machine and in a container builded with dockerTools.buildLayeredImage function.

fixes #226393
cc @adam248

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

meta.maintainers is missing

pkgs/tools/misc/tmuxifier/default.nix Outdated Show resolved Hide resolved
@dotlambda dotlambda changed the title pkgs: Add tmuxifier tmuxifier: init at 0.13.0 Sep 10, 2023
@dotlambda
Copy link
Member

Please change the commit message to the PR title.

pkgs/tools/misc/tmuxifier/default.nix Outdated Show resolved Hide resolved
@jian-lin
Copy link
Contributor

meta.maintainers is missing

I think it is optional. Could you provide some pointers to the mandatory requriement?

@dotlambda
Copy link
Member

meta.maintainers is missing

I think it is optional. Could you provide some pointers to the mandatory requriement?

It should be set: https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#meta-attributes

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/by-name/tm/tmuxifier/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/tm/tmuxifier/package.nix Show resolved Hide resolved
pkgs/by-name/tm/tmuxifier/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/tm/tmuxifier/package.nix Outdated Show resolved Hide resolved
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 10, 2023
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Sep 10, 2023
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 10, 2023
@adam248
Copy link
Contributor

adam248 commented Sep 11, 2023

Thank you all for this. ❤️

@wigust
Copy link
Contributor Author

wigust commented Sep 12, 2023

@dotlambda all your suggestions are applied. Please, let me know if something is missing.

@wigust
Copy link
Contributor Author

wigust commented Sep 14, 2023

@jian-lin @dotlambda just a friendly ping. Could you mark your requested changes as resolved, please? (all your requested changes are applied)

@dotlambda dotlambda dismissed their stale review September 14, 2023 01:53

All requested changes applied.

@dotlambda
Copy link
Member

I don't feel comfortable merging this because of the non-standard install phase. Someone should test if this actually works.

@wigust
Copy link
Contributor Author

wigust commented Sep 14, 2023

I use this package heavily in a container (builded with dockerTools.buildLayeredImage). Also, I tested that ./result/bin/tmuxifier (create session and window, starting session) works as expected.

@jian-lin jian-lin dismissed their stale review September 14, 2023 10:51

All requested changes applied.

@wigust
Copy link
Contributor Author

wigust commented Sep 16, 2023

I don't fill that this patch will ever be merged, because there are 4 peoples in the review and I don't see if anybody would like to test it.

@dotlambda Is it possible to rewrite or drop things you don't like in install phase, so it could be finally merged?

It's kinda strange for me that the proposed thing is now blocking from the merge. If we choose rewriting, how the install phase should look like? Do we need *.patch files instead of 'sed' command invocation?

@marsam marsam merged commit 5c22cac into NixOS:master Sep 21, 2023
10 checks passed
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.

Package request: tmuxifier
7 participants