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

masterpdfeditor: init at 4.2.70 #27790

Merged
merged 5 commits into from
Nov 4, 2017
Merged

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Jul 31, 2017

Motivation for this change

Added masterpdfeditor.

This is the best PDF editor available on Linux. I've tried them all but only this one allows me to add text into unfillable forms.

Things done

Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers.

  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

I had some code here for 32 bit, but since I don't have a 32 bit machine I can't test the build of it. What's the right way to specify that this package is only for 64 bit?

I tested this on nix-shell, but not part of main nixpkgs.

@CMCDragonkai CMCDragonkai force-pushed the masterpdfeditor branch 2 times, most recently from 85faa03 to 5bcd537 Compare July 31, 2017 07:17
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

  • See Spotify e.g. on how to specify the derivation is available only for one arch.
  • do test build with nix-build -A masterpdeditor. nix-shell sets some additional env vars so it is not entirely the same.
  • update the commit title according to the guidelines

url = {
"x86_64-linux" = "http://get.code-industry.net/public/master-pdf-editor-${version}_qt5.amd64.tar.gz";
}."${stdenv.system}";
sha1 = {
Copy link
Member

Choose a reason for hiding this comment

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

sha1 is no longer considered secure. Use sha256 or above.

name = "masterpdfeditor-${version}";
src = fetchurl {
url = url;
sha1 = sha1;
Copy link
Member

Choose a reason for hiding this comment

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

inherit both

@@ -13290,6 +13290,11 @@ with pkgs;

adobe-reader = callPackage_i686 ../applications/misc/adobe-reader { };

masterpdfeditor = callPackage ../applications/misc/masterpdfeditor {
Copy link
Member

Choose a reason for hiding this comment

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

I think you're supposed to use libsForQt5.callPackage

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok that fills in the qtbase and qtsvg dependencies, but something is still broken with xcb. It didn't happen with nix-shell.

let
version = "4.2.70";
url = {
"x86_64-linux" = "http://get.code-industry.net/public/master-pdf-editor-${version}_qt5.amd64.tar.gz";
Copy link
Member

Choose a reason for hiding this comment

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

get rid of the set if you only support one arch

Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to leave it out, since I can't test whether the 32 bit build works.

@CMCDragonkai CMCDragonkai force-pushed the masterpdfeditor branch 2 times, most recently from 1f029ae to c5aa656 Compare July 31, 2017 07:37
@CMCDragonkai CMCDragonkai changed the title masterpdfeditor: Added masterpdfeditor masterpdfeditor: init at 4.2.70 Jul 31, 2017
@CMCDragonkai
Copy link
Member Author

I'm getting errors like this now:

This application failed to start because it could not find or load the Qt platform plugin "xcb"
in "".

Reinstalling the application may fix this problem.
zsh: abort      masterpdfeditor4

This didn't happen inside nix-shell.

@CMCDragonkai
Copy link
Member Author

Ok looks like I might need libQt5DBus.so.5 or libqxcb.so

@CMCDragonkai
Copy link
Member Author

@FRidh So I did build it with nix-build. But with a slightly different default.nix file. And this worked without the above error.

{ pkgs ? import <nixpkgs> {} }:
  with pkgs;
  let
    version = "4.2.70";
    url = {
      "i686-linux" = "http://get.code-industry.net/public/master-pdf-editor-${version}_i386.tar.gz";
      "x86_64-linux" = "http://get.code-industry.net/public/master-pdf-editor-${version}_qt5.amd64.tar.gz";
    }."${stdenv.system}";
    sha1 = {
      "i686-linux" = "bfd8af8d68f52619504c83ea23e4dbe962a386d8";
      "x86_64-linux" = "1c7ffc8858ef5034d6f29f4ee783bc8fb4478fb5";
    }."${stdenv.system}";
    libs = {
      "i686-linux" = [
        stdenv.cc.cc
        glibc
        sane-backends
        qt4
        xorg.libXext
        xorg.libX11
        xorg.libXdmcp
        xorg.libXau
        xorg.libxcb
      ];
      "x86_64-linux" = [
        stdenv.cc.cc
        glibc
        sane-backends
        qt5.qtbase
        qt5.qtsvg
        xorg.libXext
        xorg.libX11
        xorg.libXdmcp
        xorg.libXau
        xorg.libxcb
      ];
    }."${stdenv.system}";
  in
    stdenv.mkDerivation {
      name = "masterpdfeditor-${version}";
      src = fetchurl {
        url = url;
        sha1 = sha1;
      };
      libPath = stdenv.lib.makeLibraryPath libs;
      dontStrip = true;
      installPhase = ''
        mkdir -pv $out/bin
        cp -v masterpdfeditor4 $out/bin/masterpdfeditor4
        cp -v masterpdfeditor4.png $out/bin/masterpdfeditor4.png
        cp -v -r stamps $out/bin/stamps
        cp -v -r templates $out/bin/templates
        cp -v -r lang $out/bin/lang
        cp -v -r fonts $out/bin/fonts
        patchelf --interpreter "$(cat $NIX_CC/nix-support/dynamic-linker)" \
                 --set-rpath $libPath \
                 $out/bin/masterpdfeditor4
      '';
      meta = {
        description = "Master PDF Editor";
        homepage = "https://code-industry.net/free-pdf-editor/";
        license = stdenv.lib.licenses.unfreeRedistributable;
        platforms = stdenv.lib.platforms.linux;
        maintainers = stdenv.lib.maintainers.cmcdragonkai;
      };
    }

@CMCDragonkai
Copy link
Member Author

#15498

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jul 31, 2017

#24256
#16255

@CMCDragonkai
Copy link
Member Author

According to those issues, this may not be an issue with the nix expression I wrote, but something to do with the way nixos/nixpkgs organises its qt plugins. And I'm not sure how to invocate masterpdfeditor4 with the correct qt plugins. It appears there was a fix in master to solve this somehow, but I've got a filled QT_PLUGIN_PATH env variable pointing mostly to qt4 plugins, not qt5.

@FRidh
Copy link
Member

FRidh commented Aug 1, 2017

@CMCDragonkai Qt applications cannot be run from the store anymore, but need to be installed to a profile or ran from a nix-shell. Testing it from a profile (e.g. nix-env -iA masterpdfeditor) is the right approach. I quickly installed and ran it.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Aug 1, 2017 via email

@FRidh
Copy link
Member

FRidh commented Aug 1, 2017

I meant the following doesn't work:

$(nix-build -A masterpdfeditor)/bin/masterpdfeditor4

whereas

$ nix-env -iA masterpdfeditor -f . && masterpdfeditor4

should.

Note that running it from the store did work for me, but I suppose that's because I have a Plasma desktop with the same version of Qt.

@FRidh
Copy link
Member

FRidh commented Aug 1, 2017

The underlying issue is, if I am correct, that Qt now uses propagatedUserEnvPkgs, therefore depending on packages in your profile.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Aug 2, 2017 via email

@CMCDragonkai
Copy link
Member Author

Can anybody who has more experience with QT apps review this?

@flokli
Copy link
Contributor

flokli commented Oct 16, 2017

@CMCDragonkai, @FRidh: I rebased this on latest master and tried executing, both using nix-shell and directly from the nix store. In both cases, it complained like this:

# nix-shell -p masterpdfeditor -I nixpkgs=$PWD --run "masterpdfeditor4"
This application failed to start because it could not find or load the Qt platform plugin "xcb"
in "".

Reinstalling the application may fix this problem.
/run/user/1000/nix-shell.zggzXe/rc: line 1: 29223 Aborted                 masterpdfeditor4

Did anything change by now in how QT applications should be wrapped?

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Oct 16, 2017

It did work inside a nix shell before. But I'm not sure about how Qt apps are meant to work.


EDIT: Qt apps are very side-effectful relying on some sort of plugin integration. So whether it works in nix-build, nix-env or nix-shell depends alot on the environment...

@flokli
Copy link
Contributor

flokli commented Nov 1, 2017

As described by @ttuegel in #30977, Qt applications currently don't seem to work when being run from nix-shell without a QT user environment in place, so that could have been my issue.

After having some dependent qt applications in my user environment, I was able to execute masterpdfeditor by $(nix-build -A masterpdfeditor)/bin/masterpdfeditor4, and also when adding it to my home environment (without the other qt application being pulled in).

I bumped to the latest version 4.3.61, and added myself as maintainer. Also patched and added the .desktop file from there.

@CMCDragonkai could you give my masterpdfeditor branch a try and if good push into this PR?

@CMCDragonkai
Copy link
Member Author

Your changes look good. The masterpdfeditor here does work on my machine, just not inside nix-shell, and only after I ran nix-build. So it looks like we kind of know what the problem is. Is there a way that you can push your changes into this PR? Or how would I pull in your changes into this PR into a combined PR?

@CMCDragonkai
Copy link
Member Author

I think I can pull in your branch and merge it into this PR.

@CMCDragonkai
Copy link
Member Author

@flokli Oh I can't do that, because you didn't fork from this PR, so I cannot cleanly merge your changes into this PR's branch. Instead your branch contains everything, so you should submit a PR and this PR can be closed in favour of yours.

@orivej
Copy link
Contributor

orivej commented Nov 2, 2017

@CMCDragonkai If you agree with his changes, git reset --hard his-branch and git push --force it.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Nov 2, 2017

@flokli I just tried your branch installing masterpdfeditor from there, the build worked, but running the program results in:

Cannot mix incompatible Qt library (version 0x50901) with this library (version 0x50902)
zsh: abort      masterpdfeditor4

Looks like it requires some update to Qt. My current system nixpkgs commit is 8fefa85 while your branch is later.

There's still side-effects in this Qt package that won't work unless some other environment stuff is set.

This problem exists in other Qt packages as well: #30551

So I don't think the above problem should prevent this package from being accepted. This is a global problem for all qt packages in NixOS.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Nov 2, 2017

@orivej Thanks for the tip. I did that now.

@flokli No need for extra PR. This PR is now updated with your branch.

@orivej
Copy link
Contributor

orivej commented Nov 2, 2017

It is great for PDF forms indeed. However, stamps/, templates/ etc. should go to $out/opt/masterpdfeditor/, not to $out/bin/. (For example, this is where dropbox and vagrant go.) Then $out/opt/masterpdfeditor/masterpdfeditor4 should be symlinked into $out/bin/.

@flokli
Copy link
Contributor

flokli commented Nov 2, 2017

@orivej I already tried if it could be made to load assets from $out/share/$pname, but seems it has to come from next to the binary.

I moved everything and added the symlink as suggested.

Copy link
Contributor

@orivej orivej left a comment

Choose a reason for hiding this comment

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

That's perfect, thank you!

@CMCDragonkai
Copy link
Member Author

Hey is this going to be pulled in?

@orivej orivej merged commit 86415c9 into NixOS:master Nov 4, 2017
@CMCDragonkai CMCDragonkai deleted the masterpdfeditor branch August 8, 2018 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants