duckdb: 1.3.2 -> 1.4.1; python3Packages.duckdb: 1.3.2 -> 1.4.1; python3Packages.{sqlglot,narwhals,frictionless}: fixes#444225
Merged
SuperSandro2000 merged 5 commits intoNixOS:masterfrom Oct 23, 2025
Conversation
nixpkgs-ci
bot
added
10.rebuild-linux: 101-500
Contributor
|
Hi, Have see mismatch hash : Best Regards |
nixpkgs-ci
bot
removed
the
9.needs: reviewer
Member
That hash is from nixpkgs/pkgs/development/python-modules/scipp/default.nix Lines 45 to 51 in 984cd9c |
This was referenced Sep 20, 2025
cameronraysmith
suggested changes
Sep 21, 2025
Member
There was a problem hiding this comment.
@onnimonni could you please rebase this PR after #444684 / fc1370d so we can rerun nixpkgs-review?
I went ahead and did so in #444982 for testing purposes, but it will be helpful to reveal the next error from this PR source branch on your fork.
Member
|
Based on duckdb/duckdb#18789 the current approach at nixpkgs/pkgs/development/python-modules/duckdb/default.nix Lines 28 to 40 in 50b44d8 would no longer work since tools/pythonpkg has been removed from the duckdb repo and an updated version has been placed in https://github.com/duckdb/duckdb-python.See https://github.com/cameronraysmith/nixpkgs-review-gha/actions/runs/17894997120/job/50880439985#step:6:10295 for the associated nixpkgs-review build log failure. python-duckdb build failure
python3.13-duckdb> Running phase: patchPhase
python3.13-duckdb> /nix/store/jrw7q6v8q74hhv43zgpq7i4jmxj9nwlj-stdenv-linux/setup: line 260: cd: tools/pythonpkg: No such file or directory
building '/nix/store/ach941kgb1lawmnpjrd8065x5q0bw9j0-python3.12-scipp-25.08.0.drv'...
error: Cannot build '/nix/store/ash76z93nszbyyfglzn062pw57ff2h1q-python3.13-duckdb-1.4.0.drv'.
Reason: builder failed with exit code 1.
Output paths:
/nix/store/038b8grqb2x18p0hk05lz3wv9irwn6ww-python3.13-duckdb-1.4.0
/nix/store/39cb3cjbz1wskx5wg40qm8s2zz8hq2a7-python3.13-duckdb-1.4.0-dist
Last 21 log lines:
> Sourcing python-remove-tests-dir-hook
> Sourcing python-catch-conflicts-hook.sh
> Sourcing python-remove-bin-bytecode-hook.sh
> Sourcing pypa-build-hook
> Using pypaBuildPhase
> Sourcing python-runtime-deps-check-hook
> Using pythonRuntimeDepsCheckHook
> Sourcing pypa-install-hook
> Using pypaInstallPhase
> Sourcing python-imports-check-hook.sh
> Using pythonImportsCheckPhase
> Sourcing python-namespaces-hook
> Sourcing python-catch-conflicts-hook.sh
> Sourcing pytest-check-hook
> Using pytestCheckPhase
> Running phase: unpackPhase
> unpacking source archive /nix/store/mhsl8fwkyz0x0c59a1h650w083wvcxlk-source
> source root is source
> setting SOURCE_DATE_EPOCH to timestamp 315619200 of file "source/tools/utils/test_platform.cpp"
> Running phase: patchPhase
> /nix/store/jrw7q6v8q74hhv43zgpq7i4jmxj9nwlj-stdenv-linux/setup: line 260: cd: tools/pythonpkg: No such file or directory
For full logs, run:
nix log /nix/store/ash76z93nszbyyfglzn062pw57ff2h1q-python3.13-duckdb-1.4.0.drv |
onnimonni
force-pushed
the
update-duckdb-1.4
branch
from
e78be6f to
0b23756
Compare
Contributor
Author
Hey! I think I did it but I remember sometimes it was necessary to rebase on I ran this now: $ git remote -v
origin https://github.com/onnimonni/nixpkgs.git (fetch)
origin git@github.com:onnimonni/nixpkgs.git (push)
upstream https://github.com/NixOS/nixpkgs.git (fetch)
upstream git@github.com:NixOS/nixpkgs.git (push)
$ git pull upstream master --rebase
$ git push origin HEAD --force-with-lease |
Member
|
Yes I think you got it rebased. The PR's target branch is, correctly, master so it should be rebased, as needed, on master. Since nixpkgs/pkgs/development/python-modules/duckdb/default.nix Lines 18 to 25 in 55f2008 inherits its version from nixpkgs/pkgs/by-name/du/duckdb/package.nix Lines 15 to 21 in 55f2008 the duckdb python derivation is within the blast radius of this PR and we need to decide how to address #444225 (comment) and any other issues that may have arisen in the update to 1.4.0.You can see the CI logs that result from executing nixpkgs-review, after fixing the problem with the scipp package hash, and reveal this issue here. Again, this was executed on #444982, a source branch that has a rebased copy of e78be6f on nixpkgs/master after fixing the scipp hash in #444684. |
13 tasks
Member
|
I would suggest to cleanup a bit: diff --git i/pkgs/by-name/du/duckdb/package.nix w/pkgs/by-name/du/duckdb/package.nix
index 3cfd451a4681..8a5b6700476d 100644
--- i/pkgs/by-name/du/duckdb/package.nix
+++ w/pkgs/by-name/du/duckdb/package.nix
@@ -13,7 +13,6 @@
}:
let
- enableFeature = yes: if yes then "ON" else "OFF";
versions = lib.importJSON ./versions.json;
in
stdenv.mkDerivation (finalAttrs: {
@@ -47,14 +46,12 @@ stdenv.mkDerivation (finalAttrs: {
++ lib.optionals withOdbc [ unixODBC ];
cmakeFlags = [
- "-DDUCKDB_EXTENSION_CONFIGS=${finalAttrs.src}/.github/config/in_tree_extensions.cmake"
- "-DBUILD_ODBC_DRIVER=${enableFeature withOdbc}"
- "-DJDBC_DRIVER=${enableFeature withJdbc}"
- "-DOVERRIDE_GIT_DESCRIBE=v${finalAttrs.version}-0-g${finalAttrs.rev}"
- ]
- ++ lib.optionals finalAttrs.doInstallCheck [
+ (lib.cmakeFeature "DUCKDB_EXTENSION_CONFIGS" "${finalAttrs.src}/.github/config/in_tree_extensions.cmake")
+ (lib.cmakeBool "BUILD_ODBC_DRIVER" withOdbc)
+ (lib.cmakeBool "JDBC_DRIVER" withJdbc)
+ (lib.cmakeFeature "OVERRIDE_GIT_DESCRIBE" "v${finalAttrs.version}-0-g${finalAttrs.rev}")
# development settings
- "-DBUILD_UNITTESTS=ON"
+ (lib.cmakeBool "BUILD_UNITTESTS" finalAttrs.doInstallCheck)
];
doInstallCheck = true; |
Member
|
@onnimonni if you review and merge (fast-forward only via git cli to avoid clobbering the commits and their hashes) onnimonni#1 (that PR is directly targeted into the source branch of this PR on your fork and a restricted nixpkgs-review build succeeded on all supported platforms for
Then, I think we would have what we need here to continue the discussion. If you don't want to do that for any reason, we could consider migrating the discussion to #445695, which contains the same set of changes that would arrive here after merging onnimonni#1, but it's up to you. #445695 (comment) contains instructions for downloading the |
onnimonni
force-pushed
the
update-duckdb-1.4
branch
from
0b23756 to
137d569
Compare
Contributor
Author
|
Hey @cameronraysmith I did merge the changes recommended by @sikmir above and added him as co-author for the commit. We can definitely continue in the draft PR you opened instead of using this one. I'm not sure if I understood everything you mentioned in the comment above: Should I still merge the changes from your PR to here? Also: Can I somehow give you edit access to this PR? |
Contributor
Author
|
I still ran these commands to merge your changes to this PR too: $ gh pr checkout 445695 # Your PR
$ git log -1
commit 35c1c7fa4c666f8836f4338752d7b9d5c958cb92 (HEAD -> duckdb-132-140)
Author: Cameron Smith <cameron.ray.smith@gmail.com>
Date: Tue Sep 23 14:51:20 2025 -0400
python3Packages.duckdb: 1.3.2 -> 1.4.0
$ gh pr checkout 444225 # My PR
$ git merge 35c1c7fa4c666f8836f4338752d7b9d5c958cb92
$ git push origin HEAD --force-with-lease |
nixpkgs-ci
bot
added
the
6.topic: python
onnimonni
force-pushed
the
update-duckdb-1.4
branch
from
18a6568 to
b09765b
Compare
sikmir
reviewed
Sep 25, 2025
Member
|
Hi @onnimonni: Great you got all the right changes incorporated. However, we'd need to fix the commit messages and history to match the commit conventions which would be approximately like those on #445695 (after having amended the final commit to incorporate @sikmir's comment). You can either do so via interactive rebasing of this branch
or I can help you do it if you give me write access to your nixpkgs fork in the github collaborator settings. You could likely accomplish the same that I would do from your local copy of your nixpkgs fork like ( [git remote -v] # <- check onnimonni fork remote is named origin
[git branch backup-update-duckdb-1.4 update-duckdb-1.4]
git checkout update-duckdb-1.4
git remote add cameronraysmith https://github.com/cameronraysmith/nixpkgs.git
git fetch cameronraysmith duckdb-132-140
[git reflog update-duckdb-1.4]
git reset --hard cameronraysmith/duckdb-132-140
[git reflog update-duckdb-1.4]
[git log] # <- confirm changes look sensible
git push --force-with-lease origin update-duckdb-1.4 # or replace `origin` with correct remote name of your fork
# of course you can delete backup-update-duckdb-1.4 if you created it
# but you would still be able to reset to the prior HEAD hash using
# the reflog even if you do not, which is why it's optionalIf you're comfortable with it, that would be the quickest and simplest approach. |
onnimonni
force-pushed
the
update-duckdb-1.4
branch
from
b316ec3 to
240e671
Compare
Contributor
Author
|
Thanks for the clear guide and hand holding! I really value you taking the time on involving me into your thinking process here 🙇 ❤️ And the commit messages are definitely tricky to get right here but this is so massive repo that I completely understand that strict conventions are needed. AFAIK this should be ready for reviews and merging now 👌 |
onnimonni
force-pushed
the
update-duckdb-1.4
branch
from
415fc84 to
4a28efc
Compare
Contributor
Author
|
Git rebasing is a quite complex beast. Hopefully the claude code assisted rebasing worked properly. |
Contributor
GaetanLepage
left a comment
GaetanLepage
left a comment
There was a problem hiding this comment.
You can drop the harlequin and sqlfmt commits as I fixed both packages in #454682.
Comment on lines
+71
to
+74
| preCheck = '' | ||
| export PATH="$PATH:$out/bin"; | ||
| ''; | ||
|
|
Contributor
There was a problem hiding this comment.
This is not needed as I added addBinPathToHook in nativeCheckInputs as part of #454682.
Contributor
Author
There was a problem hiding this comment.
Gotcha, I will rebase again. Thanks for the review.
Can you also guide me how I can run the nice nixpkgs-review which @cameronraysmith did above?
Contributor
Author
There was a problem hiding this comment.
I tried to now remove both of those commits. The problem is that I haven't been using git too much in this kind of way so there might be some issues.
If you want to look review the changes again it would be great 😊
Contributor
There was a problem hiding this comment.
Can you also guide me how I can run the nice
nixpkgs-reviewwhich @cameronraysmith did above?
You can either run it locally, using your own compute to build the derivations.
Simply nix-shell -p nixpkgs-review to get it. (Or add it to your packages).
If you want to test for all architectures, you might want to use nixpkgs-review-gha which lets you leverage GitHub runners.
While it supports all four main platforms, build capacity if very limited (not suited) for this PR.
Contributor
Author
There was a problem hiding this comment.
I see, thanks a lot for teaching me this 😊
I ran now these commands on my Macbook:
$ nix-shell -p nixpkgs-review
$ nixpkgs-review pr 444225 -P calibre -P unbook -P python3Packages.wacz -P py-wacz -P python3Packages.svgdigitizer -P skytemple
I have ability to simulate other platforms because I have the linux builder as well. Would that be useful?
Contributor
Author
There was a problem hiding this comment.
[0/2212 built, 16/1243/3675 copied (6978.5/37062.5 MiB), 1611.0/9286.8 MiB DL] fetching source from https:
Damn. I wish I would have purchased slightly bigger hard disk for my Macbook but there's still enough room for this.
Contributor
There was a problem hiding this comment.
Yes, this is a big PR... I am running nixpkgs-review on x86_64-linux right now.
Member
There was a problem hiding this comment.
@onnimonni, Gaétan is correct
While it supports all four main platforms, build capacity if very limited (not suited) for this PR.
What you're seeing above from me is probably quite misleading.
In general, it is NOT a great idea to try to use GHA for PRs with nix closure of this size. I've been working with Defelo to try to iron out some level of support for ping-ponging local caching up to GHA to allow you to build some fraction of the packages locally and some other fraction in GHA and slowly ratchet your way up to reviewing PRs with large closures like this one. See Defelo/nixpkgs-review-gha#49 (comment) , but note that isn't merged yet.
To replicate what I've done, you'd have to fork https://github.com/Defelo/nixpkgs-review-gha , add a git env var,
Defelo/nixpkgs-review-gha#65 (comment)
build up a cache (for now you could reuse mine, but you won't be able to push to it) and then run a command like the following
gh workflow run via local fork of Defelo/nixpkgs-review-gha `push-lltwvsmxsrsm` branch
gh workflow run review.yml \
--ref push-lltwvsmxsrsm \
-f pr=445695 \
-f x86_64-linux=false \
-f aarch64-linux=true \
-f x86_64-darwin=no \
-f aarch64-darwin=no \
-f push-to-cache=true \
-f post-result=true \
-f on-success=nothing \
-f extra-args="-P calibre -P unbook -P python3Packages.wacz -P py-wacz -P python3Packages.svgdigitizer -P skytemple"
As you can see, this probably isn't ready for "primetime" yet ;).
Hopefully this description will avoid confusing anyone else as well.
However, if this becomes a bit smoother in the future, it might be able to become part of a helpful pattern.
Member
There was a problem hiding this comment.
@onnimonni if we end up retaining the three additional fixes after having dropped sqlfmt and harlequin, maybe you should change the PR title to
duckdb: 1.3.2 -> 1.4.1; python3Packages.duckdb: 1.3.2 -> 1.4.1; python3Packages.{sqlglot,narwhals,frictionless}: fixes
so it will be easily searchable in the future.
nixpkgs-ci
bot
removed
the
2.status: merge conflict
onnimonni
force-pushed
the
update-duckdb-1.4
branch
from
4a28efc to
3efc023
Compare
…Click 8.2.1 Disable console CLI test paths due to CliRunner output capture issues: - frictionless/console/__spec__/test_console.py (2 tests) - frictionless/console/commands/__spec__/test_summary.py (3 tests) These tests expect error messages in result.stdout but it's empty, likely due to Click CliRunner API changes in how output is captured or redirected. All 1676 functional tests pass, including: - DuckDB adapter tests (frictionless/formats/sql/__spec__/duckdb/) - All other format adapters - Data validation and transformation tests The failure is in tests only.
cameronraysmith
force-pushed
the
update-duckdb-1.4
branch
from
3efc023 to
78b67e6
Compare
Member
Apologies @GaetanLepage if you really care about your review matching the HEAD SHA-1 I busted it to fix a typo in the commit message |
Member
This might not make sense now, but I'll say it anyway: Try learning jujutsu as a git history rewriting tutorial (alt to interactive rebasing) ;). In jj, you're always in the midst of a detached HEAD interactive rebase. Then, decide if you want to keep using git or prefer co-located jujutsu. lazygit is a good helper in between. Sorry this is way off-topic. |
Member
|
cameronraysmith
approved these changes
Oct 23, 2025
Member
There was a problem hiding this comment.
#444225 (comment)
#444225 (comment)
provisionally pending successful builds on other platforms
nixpkgs-ci
bot
added
the
12.approvals: 1
Contributor
|
onnimonni
changed the title
cameronraysmith
suggested changes
Oct 23, 2025
Member
There was a problem hiding this comment.
It's impossible to meaningfully review this PR for aarch64-darwin until
#453099 (comment)
#449539 (comment)
is resolved. Too many of the relevant packages depend on sbcl.
https://hydra.nixos.org/job/nixpkgs/trunk/sbcl.aarch64-darwin
https://hydra.nixos.org/build/310740111
https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+base%3Amaster+label%3A%224.workflow%3A+staging%22
cameronraysmith
removed
the
12.approvals: 1
nixpkgs-ci
bot
added
the
12.approvals: 1
GaetanLepage
approved these changes
Oct 23, 2025
Contributor
GaetanLepage
left a comment
GaetanLepage
left a comment
There was a problem hiding this comment.
I think that this is ready to be merged.
Member
|
Agreed. |
nixpkgs-ci
bot
added
12.approvals: 2
cameronraysmith
approved these changes
Oct 23, 2025
Comment on lines
+40
to
+43
| # patch cmake to ignore absence of git submodule copy of duckdb | ||
| substituteInPlace cmake/duckdb_loader.cmake \ | ||
| --replace-fail '"''${CMAKE_CURRENT_SOURCE_DIR}/external/duckdb"' \ | ||
| '"${duckdb.src}"' |
Contributor
There was a problem hiding this comment.
Would it be simpler to replace external/duckdb with a symlink to duckdb.src?
# The build depends on a duckdb git submodule
rm external/duckdb
ln -s ${duckdb.src} external/duckdb
13 tasks
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hey,
I'm not really familiar on how to test packages in nixpkgs but I did this:
And it worked just fine and used the new geometry column support for ducklake from the duckdb 1.4.0 version.
Anyway I would be interested to learn how to run the other tests too since I use this software all the time and would be willing to help maintaining it.
Thanks for separating the versions into the json file. It makes it very easy to update version and the hashes 👍.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.