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

Jump to bottom

hplip: fix hp-setup crash by adding proper NixOS PPD search path #108830

Merged
merged 1 commit into from Jan 10, 2021
Merged

hplip: fix hp-setup crash by adding proper NixOS PPD search path #108830

merged 1 commit into from Jan 10, 2021

Conversation

khumba
Copy link
Contributor

@khumba khumba commented Jan 9, 2021

Motivation for this change

This is to address the crash I hit recently trying to use hp-setup, details here: #21796.

From the commit description:

HPLIP's getSystemPPDs() function relies on searching for PPDs below common FHS
paths. None of these exist on NixOS, but the code assumes that at least one of
the directories will be found, and crashes when it doesn't (cups_ppd_path is
None and the code passes that to os.path.join).

A usable PPD search path for the running system on NixOS is
/var/lib/cups/path/share, so this patches the source to check this path as well.
This should fix the NixOS case and keep non-NixOS cases working too.

Disclaimer here, this fixes my problem with 20.09 and I've built my system config against master, but I haven't run a machine with master to actually test this out there. I plan to do so on a spare machine when I get time to set one up, but I'm opening this now to get some eyes on it.

I'm interested in having this backported to 20.09, if it's considered safe enough.

Thanks!

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)
    • nixos/tests/printing.nix passes with these changes applied to master.
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
    • Well it said no packages were found. I have done a full build of my own system config though, which includes hplipWithPlugin.
  • Tested execution of all binary files (usually in ./result/bin/)
    • Not all, but I tested hp-setup.
  • Determined the impact on package closure size (by running nix path-info -S before and after)
    • +24 bytes.
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from ttuegel January 9, 2021 05:49
doronbehar
doronbehar reviewed Jan 9, 2021
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

This seems ok but in general I think our way of removing such FHS paths is bad and might not be reliable in future releases...

@doronbehar doronbehar added the 9.needs: port to stable A PR needs a backport to the stable release. label Jan 9, 2021
@flokli
Copy link
Contributor

flokli commented Jan 9, 2021

Yeah, this should ideally be substituteAll on patches with placeholders, like done here:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/build-managers/bazel/bazel_3/default.nix#L185

@khumba
Copy link
Contributor Author

khumba commented Jan 9, 2021

I can't comment on all the other seding, but this would be better as a patch. I'll switch it over. Regarding substituteAll, it looks like /var/lib/cups/path is hard coded in a handful of places in Nixpkgs rather than being accessible by any specific Nix binding or NixOS option.

@khumba
hplip: fix hp-setup crash by adding proper NixOS PPD search path
60df55c 
HPLIP's getSystemPPDs() function relies on searching for PPDs below common FHS
paths.  None of these exist on NixOS, but the code assumes that at least one of
the directories will be found, and crashes when it doesn't (cups_ppd_path is
None and the code passes that to os.path.join).

A usable PPD search path for the running system on NixOS is
/var/lib/cups/path/share, so this patches the source to check this path as well.
This should fix the NixOS case and keep non-NixOS cases working too.
@doronbehar
Copy link
Contributor

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

2 packages built:
  • hplip
  • hplipWithPlugin

doronbehar
doronbehar approved these changes Jan 10, 2021
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Diff LGTM. hp-setup works both in hplipWithPlugin and hplip.

@doronbehar doronbehar merged commit 4539bf9 into NixOS:master Jan 10, 2021
@doronbehar doronbehar mentioned this pull request Jan 10, 2021
Closed
@khumba khumba deleted the hplip-ppd-search-path branch January 11, 2021 00:51
@erictapen
Copy link
Member

Backported in 40e8aae.
I build hplip to test the backport.

@erictapen erictapen added 8.has: port to stable A PR already has a backport to the stable release. and removed 9.needs: port to stable A PR needs a backport to the stable release. labels Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doronbehar doronbehar doronbehar approved these changes

@ttuegel ttuegel Awaiting requested review from ttuegel

Assignees
No one assigned
Labels
6.topic: nixos 8.has: port to stable A PR already has a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@khumba @flokli @doronbehar @erictapen