Skip to content

quickwit: 0.8.2 -> 0.8.2-unstable-2025-04-14 #394720

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TomaSajt
Copy link
Contributor

@TomaSajt TomaSajt commented Mar 30, 2025

Related #392903

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 30, 2025
@nix-owners nix-owners bot requested a review from happysalada March 30, 2025 20:17
@bengsparks
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 394720


aarch64-darwin

❌ 1 package failed to build:
  • quickwit

Copy link
Contributor

@bengsparks bengsparks left a comment

Choose a reason for hiding this comment

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

> error[E0282]: type annotations needed for `Box<_>`
>   --> /private/tmp/nix-build-quickwit-0.8.2.drv-1/quickwit-0.8.2-vendor/time-0.3.34/src/format_description/parse/mod.rs:83:9
>    |
> 83 |     let items = format_items
>    |         ^^^^^
> ...
> 86 |     Ok(items.into())
>    |              ---- type must be known at this point
>    |
>    = note: this is an inference error on crate `time` caused by an API change in Rust 1.80.0; update `time` to version `>=0.3.35` by calling `cargo update`

You'll need to apply an additional cargo patch to fix the inference error.

@TomaSajt
Copy link
Contributor Author

Ah true, that's why the cp was there originally.

Copy link
Contributor

@happysalada happysalada left a comment

Choose a reason for hiding this comment

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

The diff looks good, happy to let anyone merge provided that this builds actually.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Mar 31, 2025
@bengsparks
Copy link
Contributor

I'm seeing build failures here, but also in other cargo-related packages. I'll see if I can find something

@bengsparks
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 394720


aarch64-darwin

❌ 1 package failed to build:
  • quickwit

@bengsparks
Copy link
Contributor

There are 2 failing tests here.

quickwit> ---- rendezvous_hasher::tests::test_utils_sort_by_rendez_vous_hash stdout ----
quickwit>
quickwit> thread 'rendezvous_hasher::tests::test_utils_sort_by_rendez_vous_hash' panicked at quickwit-common/src/rendezvous_hasher.rs:67:9:
quickwit> assertion `left == right` failed
quickwit>   left: [127.0.0.1:10000, 127.0.0.3:10000, 127.0.0.2:10000, 127.0.0.4:10000]
quickwit>  right: [127.0.0.1:10000, 127.0.0.2:10000, 127.0.0.3:10000, 127.0.0.4:10000]
quickwit> note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
quickwit>
quickwit> ---- rate_limiter::tests::test_rate_limiter_acquire stdout ----
quickwit>
quickwit> thread 'rate_limiter::tests::test_rate_limiter_acquire' panicked at quickwit-common/src/rate_limiter.rs:173:9:
quickwit> assertion failed: !rate_limiter.acquire_bytes(ByteSize::kb(20))
quickwit>
quickwit>
quickwit> failures:
quickwit>     rate_limiter::tests::test_rate_limiter_acquire
quickwit>     rendezvous_hasher::tests::test_utils_sort_by_rendez_vous_hash
quickwit>
quickwit> test result: FAILED. 81 passed; 2 failed; 0 ignored; 0 measured; 2 filtered out; finished in 0.51s

Can you have a look at these and determine if these tests have been disabled / fixed / are patchable etc.?

The first failure is reproduced on hydra, but the second one seems new.

@bengsparks
Copy link
Contributor

bengsparks commented Apr 9, 2025

@happysalada I've been following up on this, partially due to work.
I now see that the first test failing is known, both in nixpkgs and quickwit.

I've tried my damndest to apply the relevant patches, playing around with stripLen and include directives, but it seems the patches are dependent on previous commits, which means at best I'd have to point to a commit in the middle of development between versions in order for the build to pass.

As the quickwit PR shows that this is in fact a regression related to Rust 1.82+, simply disabling the test is not an option.

Is there any news as to when quickwit 0.9 will release? Otherwise I fear this package will have to remain broken until then, which would be a real shame.

@happysalada
Copy link
Contributor

Id be happy with an upgrade to un unstabke version in the meanwhile. We could just update to latest master commit for now.
Its not ideal but i fear that the next stable release could be in a while. Ive heard that they were in talks to get bought, so they probably have other things to think about.

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Apr 9, 2025

@happysalada will you handle creating the update PR? If yes, feel free to close this PR.

@happysalada
Copy link
Contributor

Sure, ill close this if i have time to make the PR this weekend

@bengsparks
Copy link
Contributor

bengsparks commented Apr 9, 2025

@happysalada they have been bought.
https://quickwit.io/blog/quickwit-joins-datadog

@TomaSajt I imagine @happysalada could cherry-pick your commits after having applied the update. Or he names you the revision hash he wants you to use, and you grant him coauthorship of the commit. I'm sure you two can work this out 😄

@happysalada
Copy link
Contributor

happysalada commented Apr 10, 2025

I personally dont care about co authorship, im willing to merge any pr that passes the build with a recent enough commit from master . no worries if no one has tje time, ill try to get to it this weekend.

@happysalada
Copy link
Contributor

Ive just tried with fetchcargovendor and it doesnt compile properly. I have to try again with the lockfile.

@TomaSajt
Copy link
Contributor Author

Added a version bump commit, it gets past build phase but I didn't have the time to do the checkphase.

@TomaSajt TomaSajt changed the title quickwit: use fetchCargoVendor, patch out deleted git repo usage quickwit: 0.8.2 -> 0.8.2-unstable-2025-04-14 Apr 14, 2025
@TomaSajt
Copy link
Contributor Author

My PC doesn't have enough resources to handle the checkphase... what do you think about disabling it?

@bengsparks
Copy link
Contributor

@TomaSajt I'm against disabling tests. quickwit is fairly complex software, and running tests should definitely be considered normal instead of throwing them away.

@TomaSajt
Copy link
Contributor Author

Alright, then.
If anyone here has the resources to run the tests and figure out if any other tests need to be disabled, please go ahead, as I cannot do it.

@bengsparks
Copy link
Contributor

@TomaSajt I have the resources, and had already tried to. Unfortunately, nodejs_22 is failing on hydra, which means that this package does not build, as the UI depends on node. I'll post again here when that's passed 😄

@TomaSajt
Copy link
Contributor Author

Perhaps you could try nodejs_20

@happysalada
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 394720


x86_64-linux

❌ 1 package failed to build:
  • quickwit

@happysalada
Copy link
Contributor

nodejs 22 builds on master

here is the quickwit failure

failures:
quickwit>
quickwit> ---- actors::indexing_service::tests::test_indexing_service_shutdown_merge_pipeline_when_no_indexing_pipeline stdout ----
quickwit>
quickwit> thread 'actors::indexing_service::tests::test_indexing_service_shutdown_merge_pipeline_when_no_indexing_pipeline' panicked at quickwit-indexing/src/actors/indexing_service.rs:1611:9:
quickwit> assertion failed: universe.get_one::().is_none()
quickwit>
quickwit>
quickwit> failures:
quickwit> actors::indexing_service::tests::test_indexing_service_shutdown_merge_pipeline_when_no_indexing_pipeline
quickwit>

@bengsparks
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 394720


aarch64-darwin

❌ 1 package failed to build:
  • quickwit

@TomaSajt
Copy link
Contributor Author

I cannot fix the checkPhase myself as mentioned before.
If any one of you wants to invest the time into fixing it: don't use nixpkgs-review, but clone my branch and try to find which tests need to be disabled.

If you have a working build, send in the diffs and I'll apply them.

@bengsparks
Copy link
Contributor

quickwit-oss/quickwit#5746 (comment)
Perhaps we should just wait for 0.9 to come out.

@happysalada
Copy link
Contributor

Most likely whatever test we find needs disabling now, will probably also need disabling for 0.9.0.

Im happy to wait though.

@bengsparks
Copy link
Contributor

@happysalada quickwit currently targets 1.86, so I think things will work as expected; at least the test affected by the change in IpAddr Hashing will pass.
https://github.com/quickwit-oss/quickwit/blob/main/quickwit/rust-toolchain.toml

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 17, 2025
@TomaSajt TomaSajt marked this pull request as draft May 20, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants