Merged
Conversation
github-actions
bot
added
6.topic: python
Contributor
Author
|
@GaetanLepage Would you mind reviewing this? |
Contributor
|
Contributor
|
One of the python tests fail... Have you been able to build it on your end? |
Contributor
Author
|
@GaetanLepage I was able to get the Python DuckDB module's tests to pass. Just needed a couple flags changes to reflect updated flags in the upstream CI. |
GaetanLepage
previously approved these changes
Jun 4, 2025
Contributor
GaetanLepage
left a comment
GaetanLepage
left a comment
There was a problem hiding this comment.
Diff LGTM, but from my initial testing, they are a lot of packages failing. They are probably not related to this change, but it is still worth it to fix them or properly marking them as broken.
Contributor
|
Details |
Contributor
Author
|
That should've been fixed with the pytest flags I added. Are you using my latest commit? |
Contributor
Author
|
Hm, I don't see my changes here, so maybe I forgot to push them! |
cpcloud
force-pushed
the
duckdb-1.3
branch
3 times, most recently
from
00b76fd to
e47ba42
Compare
GaetanLepage
reviewed
Jun 7, 2025
Contributor
|
Contributor
|
Contributor
|
UPDATE: The following test errors are not due to your changes. Fixed in #416520
|
Contributor
|
It's time for a final bit of polish after you disable those two tests. Kindly squash and rebase your commits into a tidy sequence. It will look something like this:
|
sarahec
reviewed
Jun 13, 2025
Contributor
sarahec
left a comment
sarahec
left a comment
There was a problem hiding this comment.
Looking good! Needs a few final changes (and I suggested a couple of optional upstream patches.)
Comment on lines
40
to
41
Contributor
There was a problem hiding this comment.
This would also make a lovely upstream patch.
Contributor
Author
There was a problem hiding this comment.
There's probably a reason for the constraint here, and I don't have the time right now to go digging around in the DuckDB codebase/going through the processes to figure out why.
Contributor
There was a problem hiding this comment.
Turns out they had some build issue that caused them to set the upper bound: duckdb/duckdb@13db15b
Contributor
There was a problem hiding this comment.
Turns out that setuptools_scm 8.0, 8.1, and 8.2 all triggered the build issue, but >= 8.3 doesn't
Contributor
There was a problem hiding this comment.
Hello, my I ask a few questions for my own understanding and learning?
Was this change necessary due to failures on a specific architecture?
When I attempted this update, I only encountered problems with test/issues/general/test_17757.test, and there was no need to modify other parts of the derivation. At least on aarch64 linux and macOS.
Additionally, there is an excluded test, which is linked to an upstream issue that has now been closed, and the test passes. When should we take care of this type of cleanup? (I'm talking about test/sql/copy/file_size_bytes.test)
cpcloud
force-pushed
the
duckdb-1.3
branch
3 times, most recently
from
cfd63fb to
b26e290
Compare
Contributor
Author
|
Contributor
|
As you can see, we're making progress on your nixpkgs-review. Your change uncovered a few packages that were broken and it had a cascading effect. I'm chasing down a different bug this morning and will take a closer look at your review errors this afternoon. |
This was referenced Jul 15, 2025
Contributor
|
Update: Fixed! |
This was referenced Jul 16, 2025
Contributor
|
This was referenced Jul 17, 2025
Contributor
|
`nixpkgs-review' Failures:
|
Contributor
|
@GaetanLepage We have a handle on most of the remaining build breakages, do you want to go ahead and merge? |
Member
|
It seems the aarch64-linux build failed |
Contributor
@kuflierl did you see any logs you could share? Or the list of built/broken packages from |
Member
I rather ment the of of build at https://logs.ofborg.org/?key=nixos/nixpkgs.409698&attempt_id=1d013a3f-4664-471d-aec6-7b7b00c77922 |
Contributor
Thank you. That one test has given us trouble off and on. I'd disable it on Linux. Update: Yes, it's a known issue. Let's disable with a comment pointing to the issue. duckdb/duckdb#17757 (comment) |
Contributor
|
Member
|
FYI, this seems to have broken |
Contributor
How come we didn't catch that in |
Contributor
It did broke the package on |
13 tasks
Contributor
@vcunat thanks for flagging that. It looks one last requested change didn't get in. Fix is in #428654. |
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
6 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.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.