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.

Sign up for GitHub

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

Jump to bottom

pgadmin3: 1.22.1 -> 1.22.2 #30329

Merged
merged 1 commit into from Oct 14, 2017
Merged

pgadmin3: 1.22.1 -> 1.22.2 #30329

merged 1 commit into from Oct 14, 2017

Conversation

gleber
Copy link
Contributor

@gleber gleber commented Oct 11, 2017 •
edited

Also include Debian patch
https://sources.debian.net/data/main/p/pgadmin3/1.22.2-1/debian/patches/843344
which fixes segfault at start

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

@gleber
Copy link
Contributor Author

gleber commented Oct 11, 2017

@kuznero FYI

@kuznero
Copy link
Member

kuznero commented Oct 11, 2017

Great! Thanks! Hope it will be backported to nixos-17.03 too.

@gleber
Copy link
Contributor Author

gleber commented Oct 12, 2017

I'll take a look at the breakage later today. I wonder why it did build on my NixOS and it failed on Travis...

lsix
lsix reviewed Oct 12, 2017
View reviewed changes
pkgs/applications/misc/pgadmin/default.nix Outdated
};

enableParallelBuilding = true;

buildInputs = [ postgresql wxGTK openssl zlib ];

patches = [ ./843344.patch ];
Copy link
Member

@lsix lsix Oct 12, 2017 •
edited

Choose a reason for hiding this comment

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

Why dont you use fetchpatch instead ?

This would be:

  patches = [
    (fetchpatch {
     sha256 = "09hp7s3zjz80rpx2j3xyznwswwfxzi70z7c05dzrdk74mqjjpkfk";
     name = "843344.patch";
     url = "https://sources.debian.net/data/main/p/pgadmin3/1.22.2-1/debian/patches/843344";
    })
  ];

and avoids to put the patch itself within nixpkgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, great point!

@gleber gleber force-pushed the pgadmin-upgrade-fix branch 2 times, most recently from df5ac3b to 659c83c Compare October 12, 2017 17:59
@lsix
Copy link
Member

lsix commented Oct 13, 2017

@gleber thanks for the update.

The brakcage is quite odd, I can run nox-review pr 30329 successfully on a sandboxed nixos system.

@gleber
Copy link
Contributor Author

gleber commented Oct 13, 2017 via email

@gleber
Copy link
Contributor Author

gleber commented Oct 13, 2017

I just did nox-review pr 30329 on Ubuntu 17.10 and it succeeded.

Weird thing is that the build on travis contains -I/usr/include/postgresql, while on my Ubuntu and on my NixOS it has only paths to /nix/store. Another difference is that during my tests it uses Postgres 9.6 and on Travis it's Postgres 10.

@gleber
Copy link
Contributor Author

gleber commented Oct 13, 2017

I tested

nix-repl> :b pgadmin.override { postgresql = pkgs.postgresql100; }

and it builds, uses postgresql100 libraries and does not have -I/usr/include/postgresql

@gleber
Copy link
Contributor Author

gleber commented Oct 13, 2017

With a hint from of @LnL7 it became clear that it's configure script picks up impurity from the provided OS (it would be awesome if Travis builds in a sandbox too). Adding configureFlags = [ "--with-pgsql=${postgresql}" ] fixed the problem for me locally.

@gleber
Copy link
Contributor Author

gleber commented Oct 14, 2017

Please take a look, builds now pass

@lsix lsix merged commit c4160a4 into NixOS:master Oct 14, 2017
@lsix
Copy link
Member

lsix commented Oct 14, 2017

merged, Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsix lsix lsix left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gleber @kuznero @lsix