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

fedimint: init at 0.3.2-rc.0 #316309

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Jun 1, 2024

Description of changes

Add Fedimint package.

Fedimint is a module based system for building federated applications. It is designed to be a trust-minimized, censorship-resistant, and private alternative to centralized applications.

Fedimint is sizeable Rust project (>90k lines of Rust), in a big workspace that shares most dependencies. Build creates multiple binaries, intended to run on a different systems (cli, daemon, LN gateway with cli and daemon, etc.), so to avoid rebuilding the project multiple times, I went with the multiple outputs route.

I'm one of the main contributors to Fedimint and maintain our Nix Flake based CI system, etc. so I'm planning to maintain this package. Once this is accepted, I'd like to follow up with NixOS modules that are already part of our flake.

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.

@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/4046

pkgs/by-name/fe/fedimint/package.nix Outdated Show resolved Hide resolved
Comment on lines 84 to 81


# currently broken, will require some upstream fixes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# currently broken, will require some upstream fixes
# currently broken, will require some upstream fixes

Everything or only partly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small fix for a lack of HOME, but I need to find the time to verify. I'm also not sure how valuable is running our unit-tests in nixpkgs at all. We have a whole test suite with integration and e2e tests, and our unit-tests are comparatively low value in a grand scheme of things.

pkgs/by-name/fe/fedimint/package.nix Outdated Show resolved Hide resolved

FEDIMINT_BUILD_FORCE_GIT_HASH = gitRev;

PROTOC = "${protobuf}/bin/protoc";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PROTOC = "${protobuf}/bin/protoc";
PROTOC = "${buildPackages.protobuf}/bin/protoc";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand this one. Learning opportunity. Can you please explain or point me at something to read? @SuperSandro2000

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if there is any doc explaining cross in this detail but basically callPackage & mkDerivation together are splicing things into build, host and target architecture which might be equal or not. In this case we would get protobuf for the target arch (IIRC, basically the machine we are building the binary for to run) but we want the protoc that can run on the build machine.

pkgs/by-name/fe/fedimint/package.nix Outdated Show resolved Hide resolved
SystemConfiguration
];

outputs = [ "out" "fedimintCli" "fedimint" "gateway" "gatewayCli" "devimint" ];
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that overkill and just making things complicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. These are relatively heavy (because Rust, and because these binaries include a logic-heavy Fedimint client library), and Fedimint is a distributed system with multiple actors with different roles. There are clients, there are distributed peers in a federation, there are gateways, there are binaries integrating gateways with other systems as plugins, there is a sandbox testing utility, etc.

Once I settled on multiple-outputs approach, it seemed relatively simple to just throw everything in into role-specific output. IIUC, whoever wants all binaries together can just use the default output.

pkgs/by-name/fe/fedimint/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/fe/fedimint/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/fe/fedimint/package.nix Outdated Show resolved Hide resolved
cp -a $releaseDir/devimint $devimint/bin/
'';

FEDIMINT_BUILD_FORCE_GIT_HASH = gitRev;
Copy link
Member

Choose a reason for hiding this comment

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

We should just mock that to make things easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our build system can detect it automatically, but only if git is available. If that's OK, I'll just add git to buildInputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. Actually, it seems like the way this is downloaded, it's no longer git repository. OK. Will mock with 0oes.

Copy link
Member

@0x61nas 0x61nas left a comment

Choose a reason for hiding this comment

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

looks fine to me

@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-already-reviewed/2617/1777

@SuperSandro2000 SuperSandro2000 merged commit 49a0868 into NixOS:master Jun 25, 2024
25 checks passed
@dpc dpc mentioned this pull request Jun 27, 2024
13 tasks
@dpc
Copy link
Contributor Author

dpc commented Jun 27, 2024

I promised a follow-up with NixOS module and I pushed it in #322815

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants