Skip to content

Conversation

@dit7ya
Copy link
Member

@dit7ya dit7ya commented Oct 2, 2022

Description of changes

A distributed financial accounting database designed for mission critical safety and performance.

https://tigerbeetle.com/

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages 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. labels Oct 2, 2022
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1297

@afh
Copy link
Member

afh commented Feb 14, 2023

Would love to see this get merged (possibly after an update to the latest weekly snapshot). Who could get some 👀 on this, @dit7ya?

@afh
Copy link
Member

afh commented Feb 15, 2023

Thanks for updating this PR, @dit7ya.

Somewhat randomly pinging you, @trofi and @winterqt, as you've been very helpful with the texinfo PR #215699 and might be able to advise on how to proceed with this PR and possibly get the right people involved.

@dit7ya
Copy link
Member Author

dit7ya commented Feb 15, 2023

@afh This needs to wait until #214440 gets merged as the issue is not with Zig, rather than Tigerbeetle.

@winterqt
Copy link
Member

@afh This needs to wait until #214440 gets merged as the issue is not with Zig, rather than Tigerbeetle.

I assume you meant to say that the issue is with Zig, and not Tigerbeetle? :) Just to clarify.

Either way, we can definitely still land this, no matter the state of the Zig building situation, because you pass -Dcpu=baseline. Is there a reason you think otherwise?

In the next day or so, assuming the author of that PR doesn't respond to my comment, I'm going to PR a fix for the reproducibility of the Zig compiler itself (not what it creates), but again, that's not required to land this specific package at all.

(Let me know if any of that isn't clear.)

@afh
Copy link
Member

afh commented Feb 15, 2023

Thanks @dit7ya and @winterqt for your helpful comments and the link to the related Zig issue.

Copy link
Contributor

@trofi trofi left a comment

Choose a reason for hiding this comment

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

I think the PR looks very reasonable. I added a few convention and stylistic suggestions.

@dit7ya
Copy link
Member Author

dit7ya commented Feb 15, 2023

Thanks @trofi for the review, updated the PR with those changes.

@winterqt Hmmm, then I am not sure why the build is failing with zig from current master and not with 0.9.x?

@winterqt
Copy link
Member

winterqt commented Feb 15, 2023

Hmmm, then I am not sure why the build is failing with zig from current master and not with 0.9.x?

From the OfBorg statuses I saw for the previous run, the build was succeeding on the platforms I'd expect it to, though I didn't look at the failures. I'll wait for this run to complete, then I can give you a more complete answer :)

@afh
Copy link
Member

afh commented Feb 15, 2023

Nice to see all the progress here. Out of curiosity: do folks see value in adding the tigerbeetle clients to nixpkgs? If yes, where would these need to be added? I'm personally particularly interested in the Go and C clients.

I see that adding tigerbeetle-node would require use of node2nix, which I'm unfortunately unfamiliar with currently.

@trofi
Copy link
Contributor

trofi commented Feb 15, 2023

The default.nix looks great!

When merged to master the build fails for me as:

$ nix build --no-link github:NixOS/nixpkgs/pull/194099/merge#tigerbeetle -L
tigerbeetle> unpacking sources
tigerbeetle> unpacking source archive /nix/store/3nsdc1jri7nz8r6af0v8lipywlc5f0nk-source
tigerbeetle> source root is source
tigerbeetle> patching sources
tigerbeetle> configuring
tigerbeetle> no configure script, doing nothing
tigerbeetle> building
tigerbeetle> LLVM Emit Object... Semantic Analysis [1751] Semantic Analysis [1819] dirname... Semantic Analysis [2046] ensureTotalCapacity... Semantic Analysis [2196] btver1... Semantic Analysis [2252] nextLevel..
. Semantic Analysis [2570] avrxmega3... Semantic Analysis [2768] cortex_a53... Semantic Analysis [2969] lenAsc... Semantic Analysis [3089] usize_bits... Semantic Analysis [3187] allocatedSlice... Semantic Analysis
 [3234] Semantic Analysis [3258] growIfNeeded... Semantic Analysis [3315] keywords... LLVM Emit Object... LLVM Emit Object... Semantic Analysis [3350] /build/source/src/lsm/table.zig:97:19: error: declaration 'usa
ge' shadows function parameter from outer scope
tigerbeetle>         pub const usage = usage;
tigerbeetle>                   ^~~~~
tigerbeetle> /build/source/src/lsm/table.zig:84:14: note: previous declaration here
tigerbeetle>     comptime usage: TableUsage,
tigerbeetle>              ^~~~~
tigerbeetle> /build/source/src/lsm/table.zig:1029:9: error: pointless discard of local constant
tigerbeetle>     _ = Table;
tigerbeetle>         ^~~~~

It seems to be incompatible with most recent zig-0.10 release. If I try to pin to zig-0.9 it does work:

$ nix build --no-link --impure --expr 'with import (builtins.getFlake "github:NixOS/nixpkgs/pull/194099/merge") {}; tigerbeetle.override { zig = zig_0_9; }'
<ok!>

You can fix is by changing zig to zig_0_9 in your current default.nix. Even better would be to follow up upstream to port to latest zig if it's easy.

Copy link
Contributor

@trofi trofi left a comment

Choose a reason for hiding this comment

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

Perfect!

Out of curiosity, do you use it yourself? Do you plan to write a nixos module to make it usable as a service? (AFAIU it's a requirement to be able to use it as is).

, fetchFromGitHub
, zig_0_9
}:
stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the finalAttrs pattern here.

The finalAttrs pattern will let you remove the rec keyword (see its implementation in Nix).

Why is this pattern preferred to rec ?

Let's take this simple code example:

mkDerivation rec {
   foo = 1;
   bar = foo + 1;
}

and then .overrideAttrs(old: { foo = 2; }), you'll get { foo = 2; bar = 2; } while with finalAttrs pattern, it would work correctly because it's a real fixed point.

Let me share a couple of useful links regarding the finalAttrs pattern:

  1. History: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293
  2. Documentation: https://nixos.org/manual/nixpkgs/unstable/#mkderivation-recursive-attributes
  3. Recent example of implementation: https://github.com/NixOS/nixpkgs/compare/17f96f7b978e61576cfe16136eb418f74fab9952..9e6ea843e473d34d4f379b8b0d8ef0425a06defe

Feel free to reach out if you need some assistance.

runHook postInstall
'';

meta = with lib; {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove with lib;

@drupol drupol marked this pull request as draft August 6, 2023 11:58
@DanielSidhion DanielSidhion mentioned this pull request Dec 20, 2023
13 tasks
@ghost
Copy link

ghost commented Dec 29, 2023

superseded by #275556

@ghost ghost closed this Dec 29, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages 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. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants