Skip to content

paperjam: init at 1.2#216782

Closed
vojta001 wants to merge 1 commit intoNixOS:masterfrom
vojta001:paperjam
Closed

paperjam: init at 1.2#216782
vojta001 wants to merge 1 commit intoNixOS:masterfrom
vojta001:paperjam

Conversation

@vojta001
Copy link
Contributor

Description of changes

Paperjam is a simple command line tool for manipulating PDF files (rotate pages, reorder them, put multiple pages on one for cheaper printing…). It is similar to pspdftool, but maintained (can correctly handle compressed PDFs for example).

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.05 Release Notes (or backporting 22.11 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.

@vojta001
Copy link
Contributor Author

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

1 package built:
  • paperjam

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 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. labels Feb 17, 2023
Comment on lines 22 to 23
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"DESTDIR=$(out)"
"PREFIX=/"
"PREFIX=$(out)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like it better represents the intent, PREFIX saying don't put it under /usr but straight under / and DESTDIR saying do a "chroot" to $out during install, but if you prefer brevity, I'll change it.

Copy link
Member

@AndersonTorres AndersonTorres Feb 18, 2023

Choose a reason for hiding this comment

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

In Nixpkgs we don't use DESTDIR.

DESTDIR exists primarily for "targz compressers" used by other distros. With it, the Makefile compiles the project as if it would be installed under PREFIX (including possible hardcoded relationships like the place of libraries and configfiles), but throws the files in a DESTDIR so that the distro packagers can post-process it (including metadata, rpm style).

This is called the poor boi's chroot. (as you said)

However nix installs its things directly in the filesystem (under /nix/store/<hash>-...). We don't targz things.

Therefore, using DESTDIR here is not only superfluous, it is fundamentally wrong.

Comment on lines 37 to 39
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
license = licenses.gpl2Plus;
platforms = platforms.all;
maintainers = with maintainers; [ vojta001 ];
license = lib.licenses.gpl2Plus;
maintainers = with lib.maintainers; [ vojta001 ];
platforms = lib.platforms.all;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll fix it

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = with lib; {
meta = {

Comment on lines 9 to 11
Copy link
Member

@jtojnar jtojnar Mar 2, 2023

Choose a reason for hiding this comment

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

Looking at the Makefile, the binary would hardcode a tag name when built from a tag. Might as well use

Suggested change
# by recording the exact commit, we can embed it into the binary
# this can't be done automatically as fetchgit removes .git directory
rev = "90173f436b39bfe39491aa356a218ac185618d1c";
rev = "v${finalAttrs.version}";

That will also make it easier to update the package with passthru.updateScript = gitUpdater { }; or other similar tooling.

@dotlambda dotlambda changed the title paperjam: init paperjam: init at 1.2 Mar 3, 2023
@vojta001
Copy link
Contributor Author

vojta001 commented Aug 8, 2023

Sorry for the delay, can we merge now?

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 8, 2023
@AndersonTorres
Copy link
Member

AndersonTorres commented Aug 8, 2023

  1. Fix the merge conflicts.
  2. Because of some dark magic under the corners, the doc generation is failing over all Nixpkgs. Please wait a little more until the issue treewide: stop using types.string #247937 is solved.

Apologies for the failure.

@AndersonTorres
Copy link
Member

Now you can restart, @vojta001

@vojta001
Copy link
Contributor Author

Thank you, rebased to master.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 10, 2023
Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

I will not focus on formatting issues this time.

'';
license = lib.licenses.gpl2Plus;
maintainers = with lib.maintainers; [ vojta001 ];
platforms = lib.platforms.all;
Copy link
Member

Choose a reason for hiding this comment

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

It is not building on Darwin. The OfBorg logs accuse the lack of iconv.

@avdv
Copy link
Member

avdv commented Jan 3, 2024

@vojta001 are you still interested pushing this forward? It would be nice to have 👍

FTR, some previous work: https://github.com/jvns/nixpkgs/blob/22b70a48a797538c76b04261b3043165896d8f69/paperjam.nix (blog post: https://jvns.ca/blog/2023/02/28/some-notes-on-using-nix/)

@AndersonTorres
Copy link
Member

Closing as dead.

@avdv avdv mentioned this pull request Apr 9, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants