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

lianad: init at 6.0 #323696

Merged
merged 3 commits into from
Sep 21, 2024
Merged

lianad: init at 6.0 #323696

merged 3 commits into from
Sep 21, 2024

Conversation

plebhash
Copy link
Contributor

@plebhash plebhash commented Jul 1, 2024

Description of changes

the liana package currently only supports the liana-gui executable, while it is desired to also support headless use-cases

this PR adds a withGui attribute, which is used to choose between:

  • provide liana-gui executable if withGui is true
  • provide lianad + liana-cli executables if withGui is false (plus a $out/etc/liana/config.toml based on lianad_config_example.toml)

cc @dunxen

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.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jul 1, 2024
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jul 1, 2024
@ofborg ofborg bot requested a review from dunxen July 1, 2024 03:11
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jul 1, 2024
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Thanks for opening this!

Left some comments, and it seems that only lianad gets built without liana-cli when withGui is false.

pkgs/by-name/li/liana/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/liana/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/liana/default.nix Outdated Show resolved Hide resolved
@plebhash

This comment was marked as resolved.

pkgs/by-name/li/liana/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/liana/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/liana/default.nix Outdated Show resolved Hide resolved
@dunxen
Copy link
Contributor

dunxen commented Jul 1, 2024

it seems that only lianad gets built without liana-cli when withGui is false

@dunxen are you sure? this is what I get here:

$ nix-build -A lianad
/nix/store/jjldm51jm0jss2lnac841v4725hbjcg1-lianad-5.0
$ tree result        
result
├── bin
│   ├── liana-cli
│   └── lianad
└── etc
    └── liana
        └── config.toml

4 directories, 3 files

You are correct. I mischecked the result.

@plebhash plebhash force-pushed the lianad-lianacli branch 2 times, most recently from bfb4fc3 to 93de3eb Compare July 2, 2024 00:45
@plebhash plebhash requested a review from dunxen July 2, 2024 00:45
@plebhash plebhash force-pushed the lianad-lianacli branch 3 times, most recently from 26e0786 to 481b8c8 Compare July 2, 2024 01:03
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Oops. Didn't see the PR title (I cannot edit it). Should be:

lianad: init at 5.0

@plebhash plebhash changed the title add lianad + liana-cli executable options to liana package lianad: init at 5.0 Jul 2, 2024
@plebhash plebhash requested a review from dunxen July 2, 2024 12:38
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Copy link
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

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

it looks a bit weird because you are effectively making two derivation (which are different except for version and source.) into one. This may not be a good idea.

Good idea:

  1. Use callPackages to declare multiple packages.
  2. Just build them separately, and add a comment to remind keeping version the same
  3. Same to 2 but add assert to keep version the same. This may break r-ryantm, however

@dunxen
Copy link
Contributor

dunxen commented Jul 2, 2024

it looks a bit weird because you are effectively making two derivation (which are different except for version and source.) into one. This may not be a good idea.

Good idea:

1. Use callPackages to declare multiple packages.

2. Just build them separately, and add a comment to remind keeping version the same

3. Same to 2 but add assert to keep version the same. This may break r-ryantm, however

I agree in the current state it is not as discoverable / clear in the source what's going on, but I'm not sure which of those options more closely aligns to other packages that are in the same situation (see for example lshw vs bitcoin and a few others). Seems there are a few different methods.

I'm unfortunately unaware of any documentation around this practice, but would love to keep consistent and still have this maintainable without breaking r-ryantm.

Either way, if this should be a separate package declaration, I'll help if we need to keep them in sync manually without an assert (i.e. Option 2)

@Aleksanaa
Copy link
Member

see for example lshw vs bitcoin and a few others

That's withGui, just an option to decide whether to include gui or not, and is often passed as an argument to build tools. The withGui in this package is actually building another package. They have different sourceRoots and different dependencies. Calling it withGui is quite misleading.

Yes, there are more cursed examples, see catppuccin (although I do want to remove that stuff for a long time)

@dunxen
Copy link
Contributor

dunxen commented Jul 2, 2024

That's withGui, just an option to decide whether to include gui or not, and is often passed as an argument to build tools. The withGui in this package is actually building another package. They have different sourceRoots and different dependencies. Calling it withGui is quite misleading.

Yes, there are more cursed examples, see catppuccin (although I do want to remove that stuff for a long time)

Right, got it. Thanks @Aleksanaa for clearing that up! So let's not add to the cursed list then :)

I'm inclined to go with Option 2 and have a separate package declaration in pkgs/by-name/li/lianad/package.nix. @plebhash, is this alright with you? You can add me and yourself as maintainers there. Then you can just drop the diff in all-packages.nix :)

We can investigate option 3 at some later stage.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Jul 2, 2024
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Just changing the approval status to make it clear we're splitting this package.

@dunxen dunxen requested a review from Aleksanaa July 16, 2024 12:50
pkgs/by-name/li/lianad/package.nix Show resolved Hide resolved
hash = "sha256-RkZ2HSN7IjwN3tD0UhpMeQeqkb+Y79kSWnjJZ5KPbQk=";
};

cargoSha256 = "sha256-6r8G4sO/aDO5VyJbmbdFz3W2b8ZmulXJZtnz8IAwcaU=";
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
cargoSha256 = "sha256-6r8G4sO/aDO5VyJbmbdFz3W2b8ZmulXJZtnz8IAwcaU=";
cargoHash = "sha256-6r8G4sO/aDO5VyJbmbdFz3W2b8ZmulXJZtnz8IAwcaU=";


meta = with lib; {
mainProgram = "lianad";
description = "A Bitcoin wallet leveraging on-chain timelocks for safety and recovery";
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
description = "A Bitcoin wallet leveraging on-chain timelocks for safety and recovery";
description = "Bitcoin wallet leveraging on-chain timelocks for safety and recovery";

Copy link
Member

Choose a reason for hiding this comment

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

Please delete this

maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/by-name/li/liana/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/lianad/package.nix Show resolved Hide resolved
pkgs/by-name/li/lianad/package.nix Outdated Show resolved Hide resolved
@dunxen
Copy link
Contributor

dunxen commented Aug 3, 2024

Need to run nixfmt.

Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Needs rebase and should init at 6.0 now. I had to update liana as 5.0 had build errors with rust 1.80: #333059

@donovanglover donovanglover added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 14, 2024
@plebhash plebhash force-pushed the lianad-lianacli branch 2 times, most recently from 6916e8d to a60f686 Compare August 22, 2024 01:31
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 22, 2024
@plebhash plebhash requested a review from dunxen August 22, 2024 01:51
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Thanks! Last few comments 🤞

pkgs/by-name/li/liana/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/lianad/package.nix Outdated Show resolved Hide resolved
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 7, 2024
@plebhash plebhash force-pushed the lianad-lianacli branch 2 times, most recently from f496993 to bc8b4e3 Compare September 10, 2024 01:28
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

First commit message should be `lianad: init at 5.0`

@plebhash plebhash changed the title lianad: init at 5.0 lianad: init at 6.0 Sep 16, 2024
@Aleksanaa Aleksanaa merged commit d203fb0 into NixOS:master Sep 21, 2024
29 of 30 checks passed
@plebhash plebhash deleted the lianad-lianacli branch September 21, 2024 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants