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

pdftk: reinit at 3.0.8 #75534

Merged
merged 2 commits into from Feb 18, 2020
Merged

pdftk: reinit at 3.0.8 #75534

merged 2 commits into from Feb 18, 2020

Conversation

@averelld
Copy link
Contributor

@averelld averelld commented Dec 11, 2019

Motivation for this change

I think this is basically a maintained rewrite of the previous package. See also https://gitlab.com/pdftk-java/pdftk#known-differences-with-pdftk

Independent testing would be good. I'm keeping maintainers as is, I hope that's ok. Also, supported platforms are a guess.

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)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@averelld averelld force-pushed the averelld:pdftk-new branch from 8caa356 to 3cb508c Dec 11, 2019
@ofborg ofborg bot requested a review from 7c6f434c Dec 11, 2019
@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Dec 12, 2019

Diff looks fine. I wonder if it is a good idea to have both for some time (so that there is an easy way to the old one in case something rare is found).

The succesful build that prints a ton of Java compilation errors looks scary…

Basic functionality seems to work, I tried burst, cat with multiple files and with page selection and with rotation, stamp, background

@averelld averelld force-pushed the averelld:pdftk-new branch from 3cb508c to ae7440c Dec 12, 2019
@averelld
Copy link
Contributor Author

@averelld averelld commented Dec 12, 2019

Diff looks fine. I wonder if it is a good idea to have both for some time (so that there is an easy way to the old one in case something rare is found).

I've added a copy as pdftk-legacy, maybe after the next release that can be removed.

The succesful build that prints a ton of Java compilation errors looks scary…

It worked because those wrongly encoded chars were only in comments, I've fixed the gradle call so it's not as scary now.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Dec 12, 2019

I've added a copy as pdftk-legacy, maybe after the next release that can be removed.
Yes, and it could also be made loPrio (which is redundant but signals our low desire for its continued use).

I've fixed the gradle call so it's not as scary now.

Have you updated the output hash, though?

(also, @grahamc is it even possible to start detecting this on ofBorg? first build left something with the old outputHash in the store, so the new build succeeded)

@averelld averelld force-pushed the averelld:pdftk-new branch from ae7440c to b397ad7 Dec 12, 2019
@averelld
Copy link
Contributor Author

@averelld averelld commented Dec 12, 2019

I've added a copy as pdftk-legacy, maybe after the next release that can be removed.

Yes, and it could also be made loPrio (which is redundant but signals our low desire for its continued use).

I've added the lowPrio () wrapping

I've fixed the gradle call so it's not as scary now.

Have you updated the output hash, though?

That is not needed, because those comments have no effect on the output jar.

(also, @grahamc is it even possible to start detecting this on ofBorg? first build left something with the old outputHash in the store, so the new build succeeded)

Good point, also maybe now I'm breaking auto-updates via bot with this pattern?

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Dec 13, 2019

Good point, also maybe now I'm breaking auto-updates via bot with this pattern?

Hm, I think so. Maybe this fixed-output doesn't pay off in this case…

Also, maybe add youself as a maintainer?

@averelld
Copy link
Contributor Author

@averelld averelld commented Dec 13, 2019

Ok, I double checked, ryantm actually uses TOFU :) https://github.com/ryantm/nixpkgs-update/blob/1aa24ed735ab8ce504eaa79ae34a93fe40ff230a/src/Update.hs#L188

If it looks good to you we could merge, I might be adding this way of building to other gradle derivations.

@averelld averelld force-pushed the averelld:pdftk-new branch from b397ad7 to ac51775 Dec 13, 2019
Averell Dalton
@averelld averelld force-pushed the averelld:pdftk-new branch from ac51775 to 9fdff1c Dec 13, 2019
@averelld
Copy link
Contributor Author

@averelld averelld commented Dec 13, 2019

As pointed out here, java-compiler upgrades would break this, so going back to the old perl-patchery stuff.

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Dec 14, 2019

I would just drop the old pdftk - it is unmaintained, right? No need to have both available.

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Dec 14, 2019

Furthermore I would rename it to pdftk-java and add an alias from pdftk to pdftk-java in pkgs/top-level/aliases.nix

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Dec 14, 2019

@Mic92 it is unmaintained but working and its domain area is not exactly changing match; the port's upstream says they are not yet fully confident in the port.

};

# Point to our local deps repo
gradleInit = writeText "init.gradle" ''

This comment has been minimized.

@Mic92

Mic92 Dec 14, 2019
Contributor

Maybe this gradle fetching could be factored out into a proper fetcher for re-use eventually. But this does not need to happen as part of this PR.

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Dec 14, 2019

@7c6f434c I would not call it -legacy in that case but keep the old name than and name the new one pdftk-java. This should avoid confusion. I still have a hard time to believe there have been no bugs accumulated in the last 6 years since the last release of pdftk.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Dec 14, 2019

Given this is a semiauto port, and pdftk requires gcj requiring old gcc, I prefer the middle road of moving the old version to legacy for some time.

The old pdftk definitely has not accumulated any bugs in the last 6 years, there are of course a few cornercase bugs discovered in the past 6 years, but the port hasn't yet fixed all of them, and has definitely added a few minor bugs of its own (some of those have been fixed already, but who knows how many remain).

@7c6f434c 7c6f434c merged commit 9d16539 into NixOS:master Feb 18, 2020
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
pdftk on aarch64-linux Success
Details
pdftk on x86_64-linux Success
Details
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Feb 18, 2020
pdftk: reinit at 3.0.8
(cherry picked from commit 9d16539)
@averelld averelld deleted the averelld:pdftk-new branch Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.