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

postgresqlPackages.timescaledb: 2.14.2 -> 2.17.2; adopt, nixfmt; postgresqlPackages.timescaledb_toolkit: 1.18.0 -> 1.19.0 #348223

Merged
merged 7 commits into from
Nov 20, 2024

Conversation

kirillrdy
Copy link
Member

@kirillrdy kirillrdy commented Oct 13, 2024

Changes:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Oct 13, 2024
@kirillrdy kirillrdy force-pushed the timescale-update branch 2 times, most recently from 60a02dc to 2f0c29c Compare October 13, 2024 19:41
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Oct 13, 2024
@github-actions github-actions bot removed the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Oct 16, 2024
@kirillrdy kirillrdy marked this pull request as ready for review October 17, 2024 08:11
@nix-owners nix-owners bot requested a review from thoughtpolice October 17, 2024 08:12
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 1, 2024
@kirillrdy kirillrdy changed the title postgresqlPackages.timescaledb: 2.14.2 -> 2.17.0; adopt, nixfmt postgresqlPackages.timescaledb: 2.14.2 -> 2.17.1; adopt, nixfmt Nov 2, 2024
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

Could you squash the two update commits + the "only supports postgresql 14" together in one commit and keep the maintainer + nixfmt commits separate?

Comment on lines 65 to 68
broken = versionOlder postgresql.version "14" ||
# timescaledb supports PostgreSQL 17 from 2.17.0 on:
# https://github.com/timescale/timescaledb/releases/tag/2.17.0
# We can't upgrade to it, yet, because this would imply dropping support for
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the whole part about versionAtLeast ... "17" and the 4 line comment now, I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 2 to 10
lib,
stdenv,
fetchFromGitHub,
cmake,
postgresql,
openssl,
libkrb5,
nixosTests,
enableUnfree ? true,
Copy link
Contributor

Choose a reason for hiding this comment

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

While you have a diff because of nixfmt anyway, could you run a sort on those lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 3, 2024
@kirillrdy
Copy link
Member Author

@wolfgangwalther going to draft until after 24.11 branch off

@kirillrdy kirillrdy marked this pull request as draft November 6, 2024 09:41
@nix-owners nix-owners bot requested a review from Ma27 November 6, 2024 09:41
@wolfgangwalther
Copy link
Contributor

There is now also an update to timescaledb_toolkit available, which adds support for PG17. I suggest you bump that here, too.

I left both of them out of #356283. I did test that the new update script there, works for timescaledb and timescaledb_toolkit, too. Thus, we will be notified of new updates in the future.

Shall we go ahead with this now?

@wolfgangwalther
Copy link
Contributor

I found the following things to note:

Those might need to be mentioned in our release notes, too?

@wolfgangwalther wolfgangwalther mentioned this pull request Nov 19, 2024
13 tasks
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Nov 19, 2024
@kirillrdy kirillrdy marked this pull request as ready for review November 19, 2024 20:04
@kirillrdy
Copy link
Member Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 348223


x86_64-linux

⏩ 4 packages marked as broken and skipped:
  • postgresql13JitPackages.timescaledb
  • postgresql13JitPackages.timescaledb-apache
  • postgresql13Packages.timescaledb
  • postgresql13Packages.timescaledb-apache
⏩ 1 package blacklisted:
  • nixos-install-tools
✅ 16 packages built:
  • postgresql14JitPackages.timescaledb
  • postgresql14JitPackages.timescaledb-apache
  • postgresql14Packages.timescaledb
  • postgresql14Packages.timescaledb-apache
  • postgresql15JitPackages.timescaledb
  • postgresql15JitPackages.timescaledb-apache
  • postgresql15Packages.timescaledb
  • postgresql15Packages.timescaledb-apache
  • postgresql16JitPackages.timescaledb
  • postgresql16JitPackages.timescaledb-apache
  • postgresql16Packages.timescaledb
  • postgresql16Packages.timescaledb-apache
  • postgresql17JitPackages.timescaledb
  • postgresql17JitPackages.timescaledb-apache
  • postgresql17Packages.timescaledb
  • postgresql17Packages.timescaledb-apache

Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

Could you squash the last commit "mention timescaledb" into the second commit "2.14.2 -> 2.17.1"?

Those changes really belong together, I think.

Comment on lines 63 to 65
broken =
versionOlder postgresql.version "14"
|| (versionAtLeast postgresql.version "17" && version == "2.14.2");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry. The last line wasn't in the diff on my last comment, but the idea is to remove the whole "it's broken with v17". It's not anymore, right?

Suggested change
broken =
versionOlder postgresql.version "14"
|| (versionAtLeast postgresql.version "17" && version == "2.14.2");
broken = versionOlder postgresql.version "14";

@wolfgangwalther
Copy link
Contributor

All good on darwin as well:

Result of nixpkgs-review pr 348223 run on aarch64-darwin 1

4 packages marked as broken and skipped:
  • postgresql13JitPackages.timescaledb
  • postgresql13JitPackages.timescaledb-apache
  • postgresql13Packages.timescaledb
  • postgresql13Packages.timescaledb-apache
16 packages built:
  • postgresql14JitPackages.timescaledb
  • postgresql14JitPackages.timescaledb-apache
  • postgresql14Packages.timescaledb
  • postgresql14Packages.timescaledb-apache
  • postgresql15JitPackages.timescaledb
  • postgresql15JitPackages.timescaledb-apache
  • postgresql15Packages.timescaledb
  • postgresql15Packages.timescaledb-apache
  • postgresql16JitPackages.timescaledb
  • postgresql16JitPackages.timescaledb-apache
  • postgresql16Packages.timescaledb
  • postgresql16Packages.timescaledb-apache
  • postgresql17JitPackages.timescaledb
  • postgresql17JitPackages.timescaledb-apache
  • postgresql17Packages.timescaledb
  • postgresql17Packages.timescaledb-apache

@wolfgangwalther
Copy link
Contributor

There is now also an update to timescaledb_toolkit available, which adds support for PG17. I suggest you bump that here, too.

Did you have a look at that, too?

Without bumping timescaledb_toolkit we can't test nixosTests.postgresql.timescaledb, I think.

@kirillrdy
Copy link
Member Author

kirillrdy commented Nov 19, 2024

There is now also an update to timescaledb_toolkit available, which adds support for PG17. I suggest you bump that here, too.

Did you have a look at that, too?

Without bumping timescaledb_toolkit we can't test nixosTests.postgresql.timescaledb, I think.

I can have a look into timescaledb_toolkit, nixosTests.postgresql.timescaledb builds, but master is still on 16.4

EDIT: i was thinking of different attribute

@philon123
Copy link

@kirillrdy you're awesome, thanks! Can I help with this somehow? I could maybe confirm it working on x86_64 with a real world use case. Not sure if I can contribute to the actual PR though.

Would it be fine to update to the newest version, v2.17.2?

Any idea if this will make NixOS 24.11?

@kirillrdy
Copy link
Member Author

@kirillrdy you're awesome, thanks! Can I help with this somehow? I could maybe confirm it working on x86_64 with a real world use case. Not sure if I can contribute to the actual PR though.

Would it be fine to update to the newest version, v2.17.2?

Any idea if this will make NixOS 24.11?

any testing is much appreciated, I only looked at whats in nixosTests.timescaledb

yes I will bump to v2.17.2

no, this will not make to NixOS 24.11 due to breaking changes, but it doesn't mean it can not be installed on nixos 24.11

@kirillrdy kirillrdy changed the title postgresqlPackages.timescaledb: 2.14.2 -> 2.17.1; adopt, nixfmt postgresqlPackages.timescaledb: 2.14.2 -> 2.17.2; adopt, nixfmt; postgresqlPackages.timescaledb_toolkit: 1.18.0 -> 1.19.0 Nov 20, 2024
@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented Nov 20, 2024

postgresql17Packages.timescaledb_toolkit doesn't build for me with this PR:

       > error: unexpected argument '--pg17' found
       >
       >   tip: a similar argument exists: '--pg16'

@kirillrdy
Copy link
Member Author

postgresql17Packages.timescaledb_toolkit doesn't build for me with this PR:

       > error: unexpected argument '--pg17' found
       >
       >   tip: a similar argument exists: '--pg16'

yes, looks like we need to bump cargo-pgrx too

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Ah right, forgot to check the v17 variant of the toolkit. Good thing I got distracted from merging I guess ;-)

@@ -77,4 +77,11 @@ in
hash = "sha256-0m9oaqjU42RYyttkTihADDrRMjr2WoK/8sInZALeHws=";
cargoHash = "sha256-9XTIcpoCnROP63ZTDgMMMmj0kPggiTazKlKQfCgXKzk=";
};

cargo-pgrx_0_12_6 = generic {
Copy link
Member

Choose a reason for hiding this comment

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

Can't we bump the alpha version above?

Copy link
Member Author

Choose a reason for hiding this comment

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

This breaks vector-rs I'll have a look into that

Copy link
Member

Choose a reason for hiding this comment

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

Ah, bummer.
Nvm, you don't have to fix it up in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also wonder what the removal policy for those is. cargo-pgrx_0_10_2 is now unused in nixpkgs, because timescaledb_toolkit was the last user. Should we remove it?

Furthermore.. cargo-pgrx is very specific to PostgreSQL and our package set, I wonder whether we should take responsibility for this with the @NixOS/postgres team as well? We could either just add to the OWNERS file directly - or move cargo-pgrx into the postgresql directory. Imho, that would be a much more specific location than tools/rust/....

Certainly out of scope here, just thinking about it while I build nixosTests.postgresql.timescaledb ;)

Copy link
Member

Choose a reason for hiding this comment

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

Should we remove it?

Go for it.
I wouldn't do it on stable, but that's not the case here.

I wonder whether we should take responsibility for this with the @NixOS/postgres team as well?

Sounds good. Do you want to file a PR for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Do you want to file a PR for that?

Yeah, I can do that.

Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 348223 run on x86_64-linux 1

4 packages marked as broken and skipped:
  • postgresql13JitPackages.timescaledb
  • postgresql13JitPackages.timescaledb-apache
  • postgresql13Packages.timescaledb
  • postgresql13Packages.timescaledb-apache
1 package blacklisted:
  • nixos-install-tools
27 packages built:
  • cargo-pgrx_0_12_6
  • postgresql13JitPackages.timescaledb_toolkit
  • postgresql13Packages.timescaledb_toolkit
  • postgresql14JitPackages.timescaledb
  • postgresql14JitPackages.timescaledb-apache
  • postgresql14JitPackages.timescaledb_toolkit
  • postgresql14Packages.timescaledb
  • postgresql14Packages.timescaledb-apache
  • postgresql14Packages.timescaledb_toolkit
  • postgresql15JitPackages.timescaledb
  • postgresql15JitPackages.timescaledb-apache
  • postgresql15JitPackages.timescaledb_toolkit
  • postgresql15Packages.timescaledb
  • postgresql15Packages.timescaledb-apache
  • postgresql15Packages.timescaledb_toolkit
  • postgresql16JitPackages.timescaledb
  • postgresql16JitPackages.timescaledb-apache
  • postgresql16JitPackages.timescaledb_toolkit
  • postgresql16Packages.timescaledb
  • postgresql16Packages.timescaledb-apache
  • postgresql16Packages.timescaledb_toolkit
  • postgresql17JitPackages.timescaledb
  • postgresql17JitPackages.timescaledb-apache
  • postgresql17JitPackages.timescaledb_toolkit
  • postgresql17Packages.timescaledb
  • postgresql17Packages.timescaledb-apache
  • postgresql17Packages.timescaledb_toolkit

nixosTests.postgresql.timescaledb also built fine for all versions.

@Ma27 Ma27 merged commit f10fc70 into NixOS:master Nov 20, 2024
17 of 18 checks passed
@wolfgangwalther
Copy link
Contributor

Hm, seems like the update for cargo-pgrx broke on darwin:

run pkg_config fail: Could not run `PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 pkg-config --libs --cflags openssl`
  The pkg-config command could not be found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants