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

Jump to bottom

lnd: 0.10.0 -> 0.10.3, enable same features as upstream #89668

Merged
merged 2 commits into from Jul 14, 2020
Merged

lnd: 0.10.0 -> 0.10.3, enable same features as upstream #89668

merged 2 commits into from Jul 14, 2020

Conversation

mmilata
Copy link
Member

@mmilata mmilata commented Jun 6, 2020

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @jonasnick @cpunk2140

@ofborg ofborg bot requested a review from kalbasit June 6, 2020 16:01
jonasnick
jonasnick approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

LGTM. Checked that the default tags are the same as in the release build and that this PR actually enables the tags in the build.

@nixbitcoin
Copy link
Contributor

Can we bump this straight to lnd 0.10.2 as only that version is currently compatible with bitcoind 0.20 https://github.com/lightningnetwork/lnd/releases/tag/v0.10.2-beta.rc2 ?

@mmilata
Copy link
Member Author

mmilata commented Jun 22, 2020

@nixbitcoin sure, can bump it to 0.10.2 when it's released, in a couple of days probably.

@nixbitcoin
Copy link
Contributor

lnd v0.10.3-beta out now. Compatible with bitcoind 0.20.

@mmilata mmilata requested a review from mmahut as a code owner July 7, 2020 11:22
@mmilata mmilata changed the title lnd: 0.10.0 -> 0.10.1, enable same features as upstream lnd: 0.10.0 -> 0.10.3, enable same features as upstream Jul 7, 2020
@mmilata
Copy link
Member Author

mmilata commented Jul 7, 2020

Bumped to 0.10.3.

nixbitcoin
nixbitcoin approved these changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@nixbitcoin nixbitcoin left a comment

Choose a reason for hiding this comment

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 8115b7552b873735e2d1d5717120465ad44512ee

Got same sha256 & vendorSha256
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEV3o0Un8+KoXoD+Fk3RH5rVMIs7oFAl8EXJAACgkQ3RH5rVMI
s7oEew//QE4+9orPkZ4pVghwhGKVfYZdD2vEfh0Au5gBD1fmkywkPeNUyAbggKrb
OHyEfdaTiTBrN1V0tyO7XXJ02CSBWqaTY6NWrHvQswRzcWXc8la8OlMPmO/wem81
2Xq+aqBQwdFz9x0hbrwJBFdw0z2UUJC/zf7wv3zuqyNtNA+v4qC1uI/dL3dc2f9H
2s9j62cy2Mi/IQViP0KcPCscRHRaLkjsyXhWxxu/j/FAAFScFRMalEh1M8dwQhgf
AKM9wMf35G0Fam1KazCo/58M3RzSTKAlMTdYyXnbcIC9aTU4OwQM+8KsXkvnfA+1
PlZ8d35KHiXHd4+/+eDGW/lFFDLSi9kyF8CvMRfWFp06pqkablkvW83is9Oe3mLA
2a0Q4rwj+Pcn2iUxmtG4TnEN43mVtpmxps9OMIKEIbpZRVIR3cS9XN6NmA2pqYe1
Dbw+ORkc4ZReUNjfMyKSnOQnNfWIzbIMNmEnYX9HonjiNhgpUQaGx84Tai1LePi3
2grFGNC0GXq9zT5yLyqEt7+59Hslga4DEBGuhm4CmSlabUpDZ4zg6FeP4RUw3pDO
o5t9+ewqQgmkE25lMRS8xUVSB1sK392TJKOFjLnbWV3/HN729b4sLJdYljmwSEFl
BF+BTgIKenqFAzJvrfqqNdH8/tB9lSIRhSNwelJ3hLv8h0GK45g=
=V0Yb
-----END PGP SIGNATURE-----

@nixbitcoin
Copy link
Contributor

nixbitcoin commented Jul 7, 2020 •
edited

Tags are not showing up under build_tags with lncli version

It's causing issues with software like lightning loop which checks build tags before starting.

jonasnick
jonasnick approved these changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

LGTM.

Not a golang expert, but looking at lnd's Makefile, it seems that in order to make the tags show up in lncli version I think we have to add a -ldflags=-X github.com/lightningnetwork/lnd/build.RawTags=... build flag (and another one for build.CommitHash to fix lnd -version)

@mmilata
lnd: build with same feature tags as upstream
f800711 
See: https://github.com/lightningnetwork/lnd/blob/v0.10.3-beta/make/release_flags.mk#L36

Also pass RawTags and GoVersion to build so that it
shows in e.g. "lncli version".
@mmilata
Copy link
Member Author

mmilata commented Jul 9, 2020

Good point. I've updated the PR to set RawTags and GoVersion. We can't reliably determine Commit and CommitHash so I think it's probably better to omit the so as not to cause confusion.

I mean Commit could be set to whatever rev we pass to fetchFromGitHub but would become inaccurate if patches is defined.

@nixbitcoin
Copy link
Contributor

build_tags and go_version work now

{
    "lncli": {
        "commit": "",
        "commit_hash": "",
        "version": "0.10.3-beta",
        "app_major": 0,
        "app_minor": 10,
        "app_patch": 3,
        "app_pre_release": "beta",
        "build_tags": [
            "autopilotrpc",
            "signrpc",
            "walletrpc",
            "chainrpc",
            "invoicesrpc",
            "watchtowerrpc"
        ],
        "go_version": "go1.14.3"
    },
    "lnd": {
        "commit": "",
        "commit_hash": "",
        "version": "0.10.3-beta",
        "app_major": 0,
        "app_minor": 10,
        "app_patch": 3,
        "app_pre_release": "beta",
        "build_tags": [
            "autopilotrpc",
            "signrpc",
            "walletrpc",
            "chainrpc",
            "invoicesrpc",
            "watchtowerrpc"
        ],
        "go_version": "go1.14.3"
    }
}

Very strong.

nixbitcoin
nixbitcoin approved these changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@nixbitcoin nixbitcoin left a comment

Choose a reason for hiding this comment

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK f800711aaa7bd6a2df4197c7d88abf19b90e6212
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEV3o0Un8+KoXoD+Fk3RH5rVMIs7oFAl8IWQQACgkQ3RH5rVMI
s7r47w/8CdSQJ0hiiiRsUD4v7OOWsknP+hAyFjR9gYyelXWEuZ0bzU5ZhOSorkpY
+V4D9mPlbpWFr2XyPcq3UwowGWRSOlcyhGNHsPrS3yGoTXMLCpFmKUOfN4jlrIHd
an/A1MvmgQqNOO6Q4yWx8xgEBHCIWf3sg9rVrFX+KhHYjMRIF/myqho9SsRcVyi6
49mYYXgvApfAj+ASn2TFJdr9sDTFhJdOdsZbcNv8DqM8YA6Rq3SD3+n3jDNnZHAy
ErGMqBpwghOQv8FftDveQf/B+Rg7Sxrdou5Vnand6VFE07WbhkbE+h/79yKgvcXH
0bngu4e8nlgWrJZZ0kSYc28xwEulgllLA0jLk0tUn+cgHkvmQ1l+W8V7AD/fGB4v
DKz1M5vi5O9Lff7BwY9tGZ/WYTFUTdIoNbjM/T8zZOAWSAQ/V+YQ9EGaabRVly9L
emm43rEuULPi1AdlJ1G2xH6J3E9ZfNSgmYgiI60aZ78t+5thGtyOZAiMWuTUpuUV
Y80uhclo596B6Bqx5/FIzX3ICXyyum13zxFg8ExFOA4lIDZIvVlaEVvr2GrThifp
mQtC4s3tga5gLSJ4+nKQpYhrbj8UKg/a6hPMIYq9wnc8gLZwlAwrHbCyNYp6nSO3
fLDicu65HaCt5G4HmkvOIuAgXYL+0da2tvhcT28KOjzY7zZL5sg=
=g2CI
-----END PGP SIGNATURE-----

@nixbitcoin nixbitcoin mentioned this pull request Jul 10, 2020
Merged
jonasnick
jonasnick approved these changes Jul 12, 2020
View reviewed changes
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Cool! Can also confirm that it's possible to change the set of tags.

@jonasnick
Copy link
Contributor

It should be noted perhaps that without this PR lnd is broken because the current version is incompatible with the bitcoind version in master (0.20).

@mmahut mmahut merged commit 87eb9e3 into NixOS:master Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonasnick jonasnick jonasnick approved these changes

@nixbitcoin nixbitcoin nixbitcoin approved these changes

@kalbasit kalbasit Awaiting requested review from kalbasit

@mmahut mmahut Awaiting requested review from mmahut mmahut is a code owner

Assignees
No one assigned
Labels
8.has: package (update) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mmilata @nixbitcoin @jonasnick @mmahut