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

pyln-spec: update to latest version of the spec. #4763

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

rustyrussell
Copy link
Contributor

This may help with our dependency hell (at least, once merged and I've uploaded to pypi!)

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

This seems to be breaking the lnprototests:

=================================== FAILURES ===================================
______________________________ test_open_channel _______________________________
[gw0] linux -- Python 3.6.14 /opt/hostedtoolcache/Python/3.6.14/x64/bin/python3

runner = <lnprototest.clightning.clightning.Runner object at 0x7f5508bdcb38>
with_proposal = <function with_proposal.<locals>._setter at 0x7f550843bd08>

    def test_open_channel(runner: Runner, with_proposal: Any) -> None:
        """Tests for https://github.com/lightningnetwork/lightning-rfc/pull/880"""
>       with_proposal(channel_type_csv)

tests/test_bolt2-30-channel_type-open-accept-tlvs.py:11: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/conftest.py:44: in _setter
    + proposal_csv))
lnprototest/namespace.py:16: in make_namespace
    ns.load_csv(csv)
../../contrib/pyln-proto/pyln/proto/message/message.py:118: in load_csv
    TlvStreamType.tlvtype_from_csv(self, parts)
../../contrib/pyln-proto/pyln/proto/message/message.py:460: in tlvtype_from_csv
    tlvstream.add_field(TlvMessageType(parts[1], parts[2]))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = FieldType(accept_channel_tlvs), field = FieldType(channel_type)

    def add_field(self, field: TlvMessageType) -> None:
        if self.find_field(field.name):
>           raise ValueError("{}: duplicate field {}".format(self, field))
E           ValueError: tlvstreamtype-accept_channel_tlvs: duplicate field tlvmsgtype-channel_type

../../contrib/pyln-proto/pyln/proto/message/message.py:442: ValueError
---------------------------- Captured stdout setup -----------------------------
Port is 35257, dir is /tmp/lnprototest-clightning-loi_9ftu/bitcoind
---------------------------- Captured stdout setup -----------------------------
Port is 42957, dir is /tmp/lnprototest-clightning-tpi2sctd/bitcoind
---------------------------- Captured stdout setup -----------------------------
Port is 44779, dir is /tmp/lnprototest-clightning-8ql9i5kx/bitcoind
__________________________ test_open_channel_bad_type __________________________
[gw1] linux -- Python 3.6.14 /opt/hostedtoolcache/Python/3.6.14/x64/bin/python3

runner = <lnprototest.clightning.clightning.Runner object at 0x7fd6d87859e8>
with_proposal = <function with_proposal.<locals>._setter at 0x7fd6d86f98c8>

    def test_open_channel_bad_type(runner: Runner, with_proposal: Any) -> None:
        """Tests for https://github.com/lightningnetwork/lightning-rfc/pull/880"""
>       with_proposal(channel_type_csv)

tests/test_bolt2-30-channel_type-open-accept-tlvs.py:81: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/conftest.py:44: in _setter
    + proposal_csv))
lnprototest/namespace.py:16: in make_namespace
    ns.load_csv(csv)
../../contrib/pyln-proto/pyln/proto/message/message.py:118: in load_csv
    TlvStreamType.tlvtype_from_csv(self, parts)
../../contrib/pyln-proto/pyln/proto/message/message.py:460: in tlvtype_from_csv
    tlvstream.add_field(TlvMessageType(parts[1], parts[2]))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = FieldType(accept_channel_tlvs), field = FieldType(channel_type)

    def add_field(self, field: TlvMessageType) -> None:
        if self.find_field(field.name):
>           raise ValueError("{}: duplicate field {}".format(self, field))
E           ValueError: tlvstreamtype-accept_channel_tlvs: duplicate field tlvmsgtype-channel_type

../../contrib/pyln-proto/pyln/proto/message/message.py:442: ValueError

@vincenzopalazzo
Copy link
Contributor

This seems to be breaking the lnprototests:

Wow, it seems that it is a spaghetti dep problem 😄, @cdecker I will try to look inside the lnprototest, and try to understand what is the error here. Hopeful before tomorrow's meeting.

@cdecker
Copy link
Member

cdecker commented Sep 5, 2021

This may help with our dependency hell (at least, once merged and I've uploaded to pypi!)

Let's formalize the rules for Python dependencies: anything in the repository should not rely on PyPI whenever possible, instead using the packages in the repo itself (allows for us to stay consistent within the repo, make atomic changes, ...), only if the python code is not managed by the repo (packages from other maintainers) should we consider using PyPI, or clone directly from their repository.

@vincenzopalazzo
Copy link
Contributor

Let's formalize the rules for Python dependencies: anything in the repository should not rely on PyPI whenever possible, instead of using the packages in the repo itself (allows for us to stay consistent within the repo, make atomic changes, ...), only if the python code is not managed by the repo (packages from other maintainers) should we consider using PyPI, or clone directly from their repository.

I see we should pip3 install -e . in the correct order, and avoid a call inside the GH env about the pip install -r requirements.txt. I catch your suggestion @cdecker? maybe we can open an issue and assign it to me?

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM 2b19b69

Before ack this PR I need to update the version of lnprototest.

@rustyrussell
Copy link
Contributor Author

Yes, I've fixed lnprototest, then I can upgrade that, then this ..

Gah!

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Only the @cdecker suggestion is to be applied here.

The GH failure looks like an unlucky failure (but unable to understand way, somethings with concurrency)

test_open_opener_no_input failed; it passed 0 out of the required 1 times.
	<class 'pyln.client.lightning.RpcError'>
	RPC call failed: method: fundchannel, payload: {'id': '02c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5', 'amount': 999877, 'feerate': '253perkw', 'announce': True}, error: {'code': 303, 'message': 'Error broadcasting funding tx: error code: -27\\\\nerror message:\\\\nTransaction already in block chain. Unsent tx discarded 020000000001012f144a38afb7c3886d18f5283a8da92e79c7f6a24a64a5c7f9d5187ac2753f360400000000fdffffff02c5410f0000000000220020c46bf3d1686d6dbb2d9244f8f67b90370c5aa2747045f1aeccb77d8187117382a1c62d0000000000160014d640ab16f347d1de5aba5a715321a5fc4ba9a5d502473044022070fc67d08f2ea1bc7701337293765bb7c1daa774c2e7b500c469bad339d01921022011478ca0572817db9ffcf9440f73ef7989c37c279842c42b537ba96545f188f00121026957e53b46df017bd6460681d068e1d23a7b027de398272d0b15f59b78d060a966000000.', 'data': {'id': '02c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5', 'method': 'openchannel_signed'}}

@rustyrussell rustyrussell requested a review from cdecker September 7, 2021 02:18
Changelog-Changed: pyln-spec: updated to latest BOLT versions.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ack 11f0634

@rustyrussell rustyrussell merged commit 6b8d3a0 into ElementsProject:master Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants