Navigation Menu

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

postman: 7.0.7 -> 7.6.0 #67115

Merged
merged 2 commits into from Sep 5, 2019
Merged

Conversation

evanjs
Copy link
Member

@evanjs evanjs commented Aug 20, 2019

Motivation for this change

https://www.getpostman.com/downloads/release-notes#changelog-linux-app-7.6.0

Things done
Major refactor
  • This was primarily based on the GitKraken expression

  • Remove gnome2 (Get rid of gnome2 package set #39976)
    • Use pango instead of gnome2.pango
    • Remove gnome2.GConf
    • Remove gtk2-x11
  • Add at-spi2-atk dependency
  • Explicitly import packages rather than just pkgs or xorg
  • Refactor patchelf to be more generic
  • Use wrapGAppsHook
  • Move libPath definition out of preFixup
  • Remove dontPatchELF
  • Remove unnecessary Postman binary, only use _Postman

Testing
  • Open app
  • Sent GET and POST requests and received responses
  • Changed some settings
  • Created a new collection
  • Created a new request

Not experiencing any issues from the little I've tested so far.


  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 @xurei

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM (A few nitpicks)
binary seems to work
leaf package

pkgs/development/web/postman/default.nix Outdated Show resolved Hide resolved
pkgs/development/web/postman/default.nix Outdated Show resolved Hide resolved
pkgs/development/web/postman/default.nix Outdated Show resolved Hide resolved
pkgs/development/web/postman/default.nix Show resolved Hide resolved
pkgs/development/web/postman/default.nix Outdated Show resolved Hide resolved
@jonringer
Copy link
Contributor

oh yea, stdenv.cc.cc, guess you will need stdenv :(. Good atempt though :)

you could try to do ORIGIN=$(patch-elf --print-rpath $file); patch-elf --set-rpath "libPath:$ORIGIN" $file but that's kind of ugly, but it would prevent you from destroying the default patching.

@evanjs
Copy link
Member Author

evanjs commented Aug 21, 2019

oh yea, stdenv.cc.cc, guess you will need stdenv :(. Good atempt though :)

you could try to do ORIGIN=$(patch-elf --print-rpath $file); patch-elf --set-rpath "libPath:$ORIGIN" $file but that's kind of ugly, but it would prevent you from destroying the default patching.

Not sure if I'm reading this correctly, but it doesn't seem to work

diff --git a/pkgs/development/web/postman/default.nix b/pkgs/development/web/postman/default.nix
index b56cdf6c811..15038bdfbc7 100644
--- a/pkgs/development/web/postman/default.nix
+++ b/pkgs/development/web/postman/default.nix
@@ -84,7 +84,7 @@ stdenv.mkDerivation rec {
     patchelf --set-interpreter "$(cat $NIX_CC/nix-support/dynamic-linker)" _Postman
 
     for file in $(find . -type f \( -name \*.node -o -name _Postman -o -name \*.so\* \) ); do
-      patchelf --set-rpath ${libPath}:$out/share/postman $file || true
+      ORIGIN=$(patchelf --print-rpath $file); patchelf --set-rpath "libPath:$ORIGIN" $file || true
     done
     popd
   '';

./result/bin/postman: error while loading shared libraries: libgtk-3.so.0: cannot open shared object file: No such file or directory

Furthermore, is breaking the default patching even a concern with binary packages like this?

@jonringer
Copy link
Contributor

jonringer commented Aug 21, 2019

./result/bin/postman: error while loading shared libraries: libgtk-3.so.0: cannot open shared object file: No such file or directory

I'm not sure given the information, you can use ldd on a .so or elf to see where it did or didn't a given link. You may want to try dontPatchELF = true;, this will prevent the default phases from trying to reset rpath.

Furthermore, is breaking the default patching even a concern with binary packages like this?

I find that most pre-built binaries are usually patched in this way :(.

@evanjs
Copy link
Member Author

evanjs commented Aug 21, 2019

Ah. I think I just mistyped some things in my initial attempt based on your suggestions.
Had to move the makeLibraryPath to postFixup rather than preFixup, but everything seems to work now.

@evanjs
Copy link
Member Author

evanjs commented Aug 29, 2019

Anything else I can do to get this merged?

@evanjs
Copy link
Member Author

evanjs commented Sep 2, 2019

I have squashed the commits and moved the addition of myself to the list of maintainers (wait, do we need to do this per package or just for top-level maintainers?)
I also bumped the package to 7.6.0.
The application seems to still work fine.

@jonringer Any other thoughts before this gets merged?

@evanjs evanjs changed the title postman: 7.0.7 -> 7.5.0 postman: 7.0.7 -> 7.6.0 Sep 2, 2019
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

You need to add various libs to buildInputs for the application to be properly wrapped.
Else, when you launch a file chooser within the app, it will crash with " No GSettings schemas are installed on the system". Note that's in pure environments.

Perhaps add libPath to buildInputs.

pkgs/development/web/postman/default.nix Outdated Show resolved Hide resolved
pkgs/development/web/postman/default.nix Outdated Show resolved Hide resolved
pkgs/development/web/postman/default.nix Outdated Show resolved Hide resolved
@evanjs
Copy link
Member Author

evanjs commented Sep 5, 2019

You need to add various libs to buildInputs for the application to be properly wrapped.
Else, when you launch a file chooser within the app, it will crash with " No GSettings schemas are installed on the system". Note that's in pure environments.

Perhaps add libPath to buildInputs.

Moving everything into buildInputs and constructing libPath off of that was one method I found worked well.
I wasn't sure how to otherwise add libPath to buildInputs without duplicating dependency references.

When attempting to remove libPath and utilize autoPatchelfHook, I get no abnormal build output, but the application dumps core before launching.

@worldofpeace Given the strange behavior when I try to use autoPatchelfHook and so on, do you have any thoughts on how the expression looks right now?

@evanjs
Copy link
Member Author

evanjs commented Sep 5, 2019

@worldofpeace Thank you! This feels a lot less messy.
Is commit cleaning all that's left, now? (e.g. one for adding me to postman maintainers, one for the package updates)

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

LGTM, verified file chooser worked with a cleared XDG_DATA_DIRS.

@worldofpeace
Copy link
Contributor

@worldofpeace Thank you! This feels a lot less messy.
Is commit cleaning all that's left, now? (e.g. one for adding me to postman maintainers, one for the package updates)

Yep, you can cleanup the git history and I'll merge 👍

- Remove gnome2 (NixOS#39976)
  - Use pango instead of gnome2.pango
  - Remove gnome2.GConf
  - Remove gtk2-x11
- Add at-spi2-atk dependency
- Explicitly import packages rather than just pkgs or xorg
- Refactor patchelf to be more generic
- Use wrapGAppsHook
  - As this app uses GTK3 for the UI, we need to use wrapGAppsHook
- Move libPath creation to postFixup
- Remove dontPatchELF
- Remove unnecessary Postman binary, only use _Postman
- Remove unnecessary makeWrapper
- Add dontConfigure
- Remove unnecessary lib 
- Move libPath inputs to buildInputs
@worldofpeace worldofpeace merged commit 4b16b7e into NixOS:master Sep 5, 2019
@evanjs evanjs deleted the feature/postman-7.5.0 branch September 9, 2019 21:10
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.

None yet

3 participants