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

make: always generate proto files sans rust #6861

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

ddustin
Copy link
Collaborator

@ddustin ddustin commented Nov 10, 2023

Generating the msggen proto files doesn’t require rust (even though it generates rust files).

Keep getting in this loop where my docs don't match CI and it's because the doc scripts were never running on my machine and I didn't enable rust.

Other's will probably run into this problem and it's quite hard to figure out.

Spotted by @niftynei and @chrisguida

@ddustin ddustin requested a review from cdecker November 10, 2023 20:57
@ddustin ddustin force-pushed the ddustin/always_gen_proto branch from b7e2a13 to 4982f95 Compare November 10, 2023 21:20
Copy link
Collaborator

@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.

#10 ERROR: process "/bin/sh -c git clone /source /repo --recursive &&     cd /repo &&     ./configure --enable-static --prefix=/usr &&     make -j $(nproc) &&     make install" did not complete successfully: exit code: 2
------
 > [builder 5/5] RUN git clone /source /repo --recursive &&     cd /repo &&     ./configure --enable-static --prefix=/usr &&     make -j $(nproc) &&     make install:
79.85 printgen wire/channel_type_printgen.h
79.94 wiregen wire/onion_wiregen.c
80.03 wiregen wire/peer_wiregen.c
80.23 wiregen gossipd/gossip_store_wiregen.c
80.35 xgettext wallet/statements_gettextgen.po
80.40 python3 -m grpc_tools.protoc -I cln-grpc/proto cln-grpc/proto/node.proto --python_out=contrib/pyln-grpc-proto/pyln/grpc/ --grpc_python_out=contrib/pyln-grpc-proto/pyln/grpc/ --experimental_allow_proto3_optional
80.43 /usr/bin/python3: Error while finding module specification for 'grpc_tools.protoc' (ModuleNotFoundError: No module named 'grpc_tools')
80.44 make: *** [Makefile:386: contrib/pyln-grpc-proto/pyln/grpc/primitives_pb2.py] Error 1
80.44 make: *** Waiting for unfinished jobs....
80.52 rm external/build-x86_64-alpine-linux-musl/libwally-core-build/src/secp256k1/libsecp256k1.la
------
Dockerfile.alpine:30
--------------------
  29 |     
  30 | >>> RUN git clone /source /repo --recursive && \
  31 | >>>     cd /repo && \
  32 | >>>     ./configure --enable-static --prefix=/usr && \
  33 | >>>     make -j $(nproc) && \
  34 | >>>     make install
  35 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c git clone /source /repo --recursive &&     cd /repo &&     ./configure --enable-static --prefix=/usr &&     make -j $(nproc) &&     make install" did not complete successfully: exit code: 2

@ddustin
Copy link
Collaborator Author

ddustin commented Nov 11, 2023

This approach might not work, need to rethink it 🤔

@niftynei
Copy link
Collaborator

Error while finding module specification for 'grpc_tools.protoc' (ModuleNotFoundError: No module named 'grpc_tools')

That's a python module import error.

@cdecker cdecker force-pushed the ddustin/always_gen_proto branch from 4982f95 to f949457 Compare January 17, 2024 12:43
Generating the msggen proto files doesn’t require rust (even though it generates rust files).

Changelog-None
@cdecker cdecker force-pushed the ddustin/always_gen_proto branch from f949457 to cf13d8c Compare February 5, 2024 09:41
@cdecker
Copy link
Member

cdecker commented Feb 5, 2024

Not really all that surprising, since the Alpine compiler test does not have the grpc tooling.

@vincenzopalazzo
Copy link
Collaborator

Do we really want to require grpc tools just for a task that is not always required?

@cdecker
Copy link
Member

cdecker commented Feb 5, 2024

I added an installation step for the python build dependencies.

@vincenzopalazzo
Copy link
Collaborator

I added an installation step for the python build dependencies.

To install the poetry dependencies we should add more dependencies inside the alpine, but I do not think it is worth due that we are just testing the compilation, maybe just pip install mako <gpc tools> should be enough?

@cdecker
Copy link
Member

cdecker commented Feb 5, 2024

To install the poetry dependencies we should add more dependencies inside the alpine, but I do not think it is worth due that we are just testing the compilation, maybe just pip install mako <gpc tools> should be enough?

So it just breaks again the next time we change the dependencies? Let's not hack things together, and actually install dependencies if we declare them as necessary to build.

@cdecker cdecker force-pushed the ddustin/always_gen_proto branch from e5b896d to 2f424d4 Compare February 5, 2024 18:15
@cdecker cdecker force-pushed the ddustin/always_gen_proto branch from 2f424d4 to 43937aa Compare February 5, 2024 18:21
@vincenzopalazzo
Copy link
Collaborator

So it just breaks again the next time we change the dependencies? Let's not hack things together, and actually install dependencies if we declare them as necessary to build.

Not sure, I think that the <gpc tools> should be a developers tool, and do not required by install core lightning from source.

If some gen file are not generated when changed this is a Makefile problems and regenerate it also when it is not needed for me is just moving the problem somewhere-else, but I am fine ether way.

@cdecker cdecker merged commit a87643f into ElementsProject:master Feb 8, 2024
36 checks passed
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.

4 participants