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

Add postgis 2.4.0 #30321

Merged
merged 1 commit into from Oct 14, 2017
Merged

Add postgis 2.4.0 #30321

merged 1 commit into from Oct 14, 2017

Conversation

RemiDesgrange
Copy link
Contributor

@RemiDesgrange RemiDesgrange commented Oct 11, 2017 •
edited by lsix

nixos/postgis: add postgis 2.4.0

doesn't remove v2.3.1. There are some big change in 2.4 that people may
don't want. see https://postgis.net/docs/release_notes.html#idm41021

Motivation for this change

Postgis v2.3.1 doesn't work with postgresql v10. Adding v2.4 add support for v10

This is my first PR here, please don't hesitate to comment. I changed a test, but I didn't know if it was better to create a new one or changing this one, since v2.2 isn't in nixpkgs anymore.

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.

@jtojnar
Copy link
Contributor

jtojnar commented Oct 11, 2017

  • The test is not defined anyway. You would have to add it to nixos/release.nix and then run nix-build -A tests.postgis.x86_64-linux nixos/release.nix --show-trace.

  • If you are not using some code, you can just delete it instead of commenting.

@RemiDesgrange
Copy link
Contributor Author

Adding this

tests.postgis = callSubTests tests/postgis.nix { system = "x86_64-linux"; };

in nixos/release.nix cause the following trace when nix-build -A tests.postgis.x86_64-linux nixos/release.nix --show-trace not sure if I did something wrong 🙀

nix-build -A tests.postgis.x86_64-linux nixos/release.nix --show-trace
error: while evaluating ‘callSubTests’ at /root/nixpkgs/nixos/release.nix:22:22, called from /root/nixpkgs/nixos/release.nix:306:19:
while evaluating ‘discover’ at /root/nixpkgs/nixos/release.nix:23:16, called from /root/nixpkgs/nixos/release.nix:33:28:
while evaluating ‘mapAttrs’ at /root/nixpkgs/lib/attrsets.nix:198:17, called from /root/nixpkgs/nixos/release.nix:25:8:
while evaluating ‘filterAttrs’ at /root/nixpkgs/lib/attrsets.nix:115:23, called from /root/nixpkgs/nixos/release.nix:24:18:
while evaluating ‘concatMap’ at /root/nixpkgs/lib/lists.nix:102:18, called from /root/nixpkgs/lib/attrsets.nix:116:18:
while evaluating anonymous function at /root/nixpkgs/lib/attrsets.nix:116:29, called from undefined position:
value is a boolean while a set was expected, at /root/nixpkgs/lib/attrsets.nix:116:62
```

@jtojnar
Copy link
Contributor

jtojnar commented Oct 11, 2017

Since it is a single test, it should be just callTest. callSubTests is for the case when there are multiple test cases (e.g. for each postgres version).

@RemiDesgrange
Copy link
Contributor Author

Hum postgresql doesn't seems it want to start. But on my machine the test/postgresql.nix doesn't work either

lsix
lsix requested changes Oct 12, 2017
View reviewed changes
Copy link
Member

@lsix lsix left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for the work!
I have added few comments here and there, but at the end it looks good and I can run the test successfully.

nixos/tests/postgis.nix Outdated
#initialScript = pkgs.writeText "postgresql-init.sql"
#''
#CREATE ROLE postgres WITH superuser login createdb;
#'';
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 those lines can be completly removed singe they are not necessary anymore.

nixos/release.nix Outdated
@@ -303,6 +303,7 @@ in rec {
#tests.panamax = hydraJob (import tests/panamax.nix { system = "x86_64-linux"; });
tests.peerflix = callTest tests/peerflix.nix {};
tests.postgresql = callSubTests tests/postgresql.nix {};
tests.postgis = callTest tests/postgis.nix { system = "x86_64-linux"; };
Copy link
Member

Choose a reason for hiding this comment

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

Do you have any particular reason to restrict to "x86_64" ? Why not letting it open:

tests.postgis = callTest tests/postgis.nix {};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the next push is ok (test, ok for you) i'll rebase

@lsix
Copy link
Member

lsix commented Oct 12, 2017

Looks good to me.

Before this is merged, you should squash those commits (note that the first of your commits have been done with another identity, you may want to fix that). Also to get closer to the contribution guidelines, the commit message should be postgis: add v_2_4_0 or something like it.

@lsix
Copy link
Member

lsix commented Oct 13, 2017

@RemiDesgrange the commit is still authored by "System administrator", is this what you want ? If yes, I’ll merge it shortly. Otherwise, git commit --amend --author="Author Name <email@address.com>" might be what you are looking for.

@RemiDesgrange
postgis: add v2.4.0
8dcaa5b 
Add postgis 2.4.0

doesn't remove v2.3.1. There are some big change in 2.4 that people may
don't want. see https://postgis.net/docs/release_notes.html#idm41021

fix test call

modify following recommandation of lsix
@RemiDesgrange
Copy link
Contributor Author

Sorry for that I changed it

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

lsix commented Oct 14, 2017

Merged, thanks !

@jtojnar jtojnar mentioned this pull request Jan 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsix lsix lsix approved these changes

Assignees
No one assigned
Labels
12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@RemiDesgrange @jtojnar @lsix @Janik-Haag