-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Add btcpayserver #95104
Add btcpayserver #95104
Conversation
e3d161d
to
cc7658f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for packaging this useful app! Got a few comments inline.
# 0. May need to run `dotnet nuget locals all --clear` if the user has built | ||
# the package with dotnet previously | ||
# 1. create a log with `dotnet restore -v n MyPackage.sln > mypackage-restore.log | ||
# 2. then call ./create-deps.sh mypackage-restore.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script might be useful for other packages, I wonder if it makes sense to move it to e.g. maintainers/scripts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of dotnet packages using a variation of this script. I took it and adjusted it slightly from one of them. Not sure where it originated though.
|
||
installPhase = '' | ||
mkdir -p $out/bin/ | ||
echo -e '#!/usr/bin/env bash \n#launch BTCpayserver \nscriptdir=`dirname $(readlink $(which btcpayserver))` \ndotnet run --no-launch-profile --no-build -c Release -p "$scriptdir/../BTCPayServer/BTCPayServer.csproj" -- $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: use writeScript (or makeWrapper?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also using readlink
makes the scriptdir dependent on $PATH
. I.e. if btcpayserver
is not in $PATH
it won't start:
$ nix-build -A btcpayserver
...
$ ./result/bin/btcpayserver
which: no btcpayserver in (/run/wrappers/bin:/home/mmilata/.nix-profile/bin:/etc/profiles/per-user/mmilata/bin:/nix/var/nix/profiles/default/bin:/run/current-system/sw/bin)
readlink: missing operand
Try 'readlink --help' for more information.
dirname: missing operand
Try 'dirname --help' for more information.
./result/bin/btcpayserver: line 4: dotnet: command not found
Can you use the value of $out
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will address this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still getting the same error:
$ ./result/bin/btcpayserver
./result/bin/btcpayserver: line 3: dotnet: command not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:/ my bad, the script was calling the dotnet binary in my environment. Fix is pushed now (with writeScript as well)
dontStrip = true; | ||
|
||
meta = with lib; { | ||
description = "BTCPay Server is a self-hosted, open-source cryptocurrency payment processor. It's secure, private, censorship-resistant and free"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description should not start with a package name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will push a fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a fix
cc7658f
to
ca4cf12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the package! I am not qualified to review dotnet packaging but is there an existing similar package already? Fwiw, I can build it on archlinux with sandbox and I can successfully play around with btcpayserver
after installing dotnet-sdk_3
. Is that dotnet-sdk_3 runtime dependency necessary? The wasabibackend package doesn't seem to require this.
Is it true that the nbxplorer package is independent and is not a dependency of btcpayserver?
script='#!/usr/bin/env bash\n#launch BTCpayserver\ndotnet run --no-launch-profile --no-build -c Release -p "REPLACE/BTCPayServer/BTCPayServer.csproj" -- $@' | ||
echo -e $script | sed "s@REPLACE@$out@g" > $out/bin/run.sh | ||
chmod +x $out/bin/run.sh | ||
ln -s $out/bin/run.sh $out/bin/btcpayserver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need both run.sh
and btcpayserver
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not needed. Removed in the latest commit
ca4cf12
to
c101ae0
Compare
I think it is since this happened #95104 (comment)
It's possible to run nbxplorer without btcpayserver as stated in the nbxplorer readme though I don't think there are many that are doing so. |
Nice, the fix works for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at all familiar with C# packaging but compiles on x86_64 and seems to start fine.
mkdir home | ||
export HOME=$PWD/home | ||
|
||
export DOTNET_CLI_TELEMETRY_OPTOUT=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this to the bin-script? Or something like DOTNET_CLI_TELEMETRY_OPTOUT=${DOTNET_CLI_TELEMETRY_OPTOUT:-1}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added DOTNET_CLI_TELEMETRY_OPTOUT=1
Module: fort-nix/nix-bitcoin#221 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK
Compiles and works as a basis for my module
c101ae0
to
47a8e94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the following changes: https://github.com/erikarvstedt/nixpkgs/commits/add-btcpayserver
See the individual commit messages for details. Highlights:
- the script can now update the pkgs to the lastest release
- ~10x faster pkg build time
- decreased pkg output size : 830M -> 6.6M (nbxplorer), 1.5G -> 77M (btcpayserver)
- much faster
deps.nix
generation
Here's the squashed branch including these changes: https://github.com/erikarvstedt/nixpkgs/commits/add-btcpayserver-squashed
Note the updated version in the nbxplorer commit message.
This includes the latest fixups from NixOS/nixpkgs#95104
Very nice improvements @erikarvstedt. Agree that they should be added to this PR. I also like that you kept the common-updater very general and simplified the build phase. My generated deps.nix differs in the order of the listed dependencies because this sort on my machine puts |
The order was produced by |
@erikarvstedt I'll just cherry-pick your squashed commits here if that's ok |
@kcalvinalvin, sure, that's what I had prepared the squashed branch for 🐱 Could you also add the latest changes? Just pull in the squashed branch again. |
Fix works for me. I can reproduce exactly the same deps.nix now. |
I rebased my module on @erikarvstedt's changes. The speed and size improvements are great. Everything compiles fine, but when I try to connect to the BTCPayServer URL I get the following error:
Could this have to do with |
@nixbitcoin I am also getting the same issue. Was able to fix the issue by reverting the In the btcpayserver repo, they just have a 3 line shell script which I just copied. I think this should just be followed? |
Pushed new commits that are based off of @erikarvstedt's work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not testing the HTTP server. I've filed an upstream bug report.
@kcalvinalvin, please add these changes. They work around the build bug while keeping the reduced output size (89M for btcpayserver) of the dotnet publish
approach.
807e089
to
4fe6bf6
Compare
Merged the new commits and checked that the webfront works |
@kcalvinalvin, thanks! |
@erikarvstedt done :) |
4fe6bf6
to
a04a87b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a04a87b
Tested as basis of fort-nix/nix-bitcoin#221
1f125cd
to
9959488
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, there were two more small fixes I asked @kcalvinalvin to pull in.
This looks great now. 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We missed adding the version
to the package names.
@kcalvinalvin, as before, please update from here.
@erikarvstedt I can do that. But I'm not seeing any version changes with
|
Yes, these changes are correct. |
9959488
to
2f55d6f
Compare
TIL. Changes applied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me 2f55d6f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2f55d6f
Thank you, great work! |
Motivation for this change
Adds package btcpayserver and its dependency, nbxplorer
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)