Skip to content

cups-pdf: misc improvements in package, module, vmtest#387364

Merged
drupol merged 3 commits into
NixOS:masterfrom
Yarny0:cups-pdf-vmtest-getexe
Apr 15, 2025
Merged

cups-pdf: misc improvements in package, module, vmtest#387364
drupol merged 3 commits into
NixOS:masterfrom
Yarny0:cups-pdf-vmtest-getexe

Conversation

@Yarny0

@Yarny0 Yarny0 commented Mar 5, 2025

Copy link
Copy Markdown
Contributor
  • cups-pdf-to-pdf package: remove with lib; from meta, drop unused rec, use lib.getExe and substituteInPlace
  • cups-pdf vm test: migrate to runTest, use lib.getExe', avoid deprecated ${pkgs.imageMagick}/bin/convert program

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD" tries to rebuild 3000+ package and was therefore skipped.


Add a 👍 reaction to pull requests you find important.

@github-actions github-actions Bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 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 Mar 5, 2025
@nixos-discourse

Copy link
Copy Markdown

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/5289

@nixos-discourse

Copy link
Copy Markdown

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/5323

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@Yarny0 Yarny0 force-pushed the cups-pdf-vmtest-getexe branch from 90b6687 to 14fe270 Compare April 2, 2025 17:20
@ofborg ofborg Bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@Yarny0

Yarny0 commented Apr 2, 2025

Copy link
Copy Markdown
Contributor Author

Pushed a rebase due to #380990

Comment thread nixos/tests/cups-pdf.nix Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

imageMagickBig.meta.mainProgram is "magick", so we can use the non-prime getExe here:

Suggested change
run(f"${lib.getExe' hostPkgs.imagemagickBig "magick"} -density 300 $out/{name}.pdf $out/{name}.jpeg", shell=True, check=True)
run(f"${lib.getExe hostPkgs.imagemagickBig} -density 300 $out/{name}.pdf $out/{name}.jpeg", shell=True, check=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be practical to use substituteInPlace and --replace-fail instead of sed here? Generally, this is preferred over using sed.

Something like:

Suggested change
sed -r 's|(gscall, size, ")cp |\1${lib.getExe' coreutils "cp"} |' cups-pdf.c -i
substituteInPlace cups-pdf.c \
--replace-fail \
'gscall, size, "cp ' \
'gscall, size, "${lib.getExe' coreutils "cp"} '

or maybe even:

Suggested change
sed -r 's|(gscall, size, ")cp |\1${lib.getExe' coreutils "cp"} |' cups-pdf.c -i
substituteInPlace cups-pdf.c \
--replace-fail \
'"cp ' \
'"${lib.getExe' coreutils "cp"} '

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Reducing the "discovery string" is a bit risky, but your suggestion still looks passable to me (I've also added an explanation to this effect to the commit message).

Yarny0 added 2 commits April 4, 2025 12:18
With `lib.getExe`, we also use "magick" instead of "convert`,
and thereby avoid the ImageMagick warning message:

> WARNING: The convert command is deprecated in IMv7, use "magick" instead of "convert" or "magick convert"

The replacement string for the
`cp` command is shortened.
This is a bit risky since an update might inadvertently
introduce a similar string that is then also replaced.
However, the leading double-quote and
trailing space seem safe enough to me.
@Yarny0 Yarny0 force-pushed the cups-pdf-vmtest-getexe branch from 14fe270 to ce5e427 Compare April 4, 2025 10:19
@Yarny0

Yarny0 commented Apr 4, 2025

Copy link
Copy Markdown
Contributor Author

Rebased and applied suggestions by @MattSturgeon . Package still builds, cups-pdf-to-pdf.tests still pass.

@github-actions github-actions Bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 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 Apr 4, 2025
@MattSturgeon MattSturgeon added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 1 This PR was reviewed and approved by one person. labels Apr 4, 2025
@MattSturgeon MattSturgeon requested a review from drupol April 4, 2025 12:35
@nixos-discourse

Copy link
Copy Markdown

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/5390

@drupol drupol merged commit e699969 into NixOS:master Apr 15, 2025
@Yarny0 Yarny0 deleted the cups-pdf-vmtest-getexe branch April 16, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants