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

yazi: clean up wrapper, add options and format #321076

Merged
merged 2 commits into from
Jul 3, 2024
Merged

Conversation

eljamm
Copy link
Contributor

@eljamm eljamm commented Jun 19, 2024

Description of changes

Add exiftool, which is needed for showing EXIF data when interactively opening images with O

  • Remove redundant withPackage options
  • Add optionalDeps for packages that provide additional features
  • Add extraPackages for user-defined packages
  • Format with nixfmt

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
eljamm Fedi Jamoussi
@eljamm
Copy link
Contributor Author

eljamm commented Jun 19, 2024

Result of nixpkgs-review pr 321076 run on x86_64-linux 1

1 package built:
  • yazi

@XYenon
Copy link
Member

XYenon commented Jun 20, 2024

I don't think we should add exiftool to the wrapper. Unlike other tools which are used to provide yazi functions in rust or lua codes, the only usage of exiftool is in the default opener config. And I don't think we should add tools only in config to the wrapper, because config can and usually be changed.

@eljamm
Copy link
Contributor Author

eljamm commented Jun 20, 2024

@XYenon As a new user, I expect things to work out of the box without me having to configure anything. Also, looking at the history of that config file, exiftool has always been there, so I don't think it's likely to change in the future.

That being said, if you still don't want to add it, how about adding an extraPackages option, instead?

That way, users will easily be able to simply add the tools they need to the wrapper.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Jun 20, 2024
@linsui
Copy link
Contributor

linsui commented Jun 20, 2024

What's the benefit to add packages to the wrapper instead of globally?

@eljamm
Copy link
Contributor Author

eljamm commented Jun 20, 2024

@linsui Well, the main benefit is making sure those packages are installed/deleted with the program and that it functions as it should out of the box.

For example, say that I want to try yazi but unar has not been added to the wrapper. This means I also need to install unar in order to preview archive files. As such, I need to remember to remove unar when I no longer want to use yazi, else I'd have a package I don't need in my PATH, which also won't be cleaned up by the GC.

Therefore, by adding the tools we need to the wrapper, we're effectively coupling them to the main program.

@XYenon
Copy link
Member

XYenon commented Jun 20, 2024

mpv, mediainfo, and even vscode are in the default openers, I don't think exiftool is special. Add a extraPackages is fine for me.

@eljamm
Copy link
Contributor Author

eljamm commented Jun 20, 2024

Alright, should I open a new PR or do I force push here? Perhaps pushing is quicker as it's not a big change anyways.

@eljamm eljamm changed the title yazi: add exiftool to wrapper yazi: add extraPackages option Jun 20, 2024
@eljamm eljamm requested a review from matthiasbeyer June 20, 2024 10:16
@eljamm
Copy link
Contributor Author

eljamm commented Jun 20, 2024

Tested the change with:

$ nix-build -E 'with import ./. {}; (yazi.override { extraPackages = [ exiftool ]; })'

And everything works correctly.

@linsui
Copy link
Contributor

linsui commented Jun 20, 2024

Maybe we should unify those options. E.g.

extraPackages ? [ file jq ... ]

@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 and removed 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 10.rebuild-linux: 1 labels Jun 20, 2024
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jun 21, 2024
@eljamm eljamm changed the title yazi: add extraPackages option yazi: clean up wrapper, add options and format Jun 21, 2024
@linsui
Copy link
Contributor

linsui commented Jun 21, 2024

How about putting file in optionalDeps and removing withRuntimeDeps?

@eljamm
Copy link
Contributor Author

eljamm commented Jun 21, 2024

According to the yazi docs, file is not optional. Not sure about withRuntimeDeps, though. How useful is it to toggle runtime deps?

@linsui
Copy link
Contributor

linsui commented Jun 21, 2024

Without file yazi can still run and browse files though previewer doesn't work. I feel that it doesn't worth adding withRuntimeDeps only for that. withRuntimeDeps becomes useless anyway.

@eljamm
Copy link
Contributor Author

eljamm commented Jun 21, 2024

I don't mind removing withRuntimeDeps if its usefulness is limited, but I prefer to keep file as a non-optional dependency since that's what upstream considers it.

Verified

This commit was signed with the committer’s verified signature.
eljamm Fedi Jamoussi
- Remove redundant `withX` options
- Add `optionalDeps`: packages that provide additional features
- Add `extraPackages`: user-defined packages
- Format with nixfmt
@eljamm
Copy link
Contributor Author

eljamm commented Jun 22, 2024

Does it make sense to add extraPackages or optionalDeps to the yazi module or should the package just be overwritten?

Also, are these changes considered a breaking API change and thus can't be backported to the stable branch?

@ofborg ofborg bot requested a review from XYenon June 22, 2024 08:41
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 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 Jun 22, 2024
@sxyazi
Copy link

sxyazi commented Jun 22, 2024

Without file yazi can still run and browse files though previewer doesn't work

It can also cause files to be unable to open - I think being able to open files is a baseline for a file manager, so it's worth mentioning here.

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jun 22, 2024
@linsui
Copy link
Contributor

linsui commented Jun 23, 2024

I don't mind removing withRuntimeDeps if its usefulness is limited, but I prefer to keep file as a non-optional dependency since that's what upstream considers it.

Sounds good to me.

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jun 24, 2024
@wegank wegank added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Jun 24, 2024
@Aleksanaa Aleksanaa merged commit 24b44a1 into NixOS:master Jul 3, 2024
30 checks passed
@eljamm eljamm deleted the yazi branch July 3, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants