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

docker: reintroduce poetry and improve the install command #5606

Conversation

vincenzopalazzo
Copy link
Collaborator

Ok! imho the problem is not poetry, the problem is why after the installation we have the problem of missing mako, However, the solution to export the requirements.txt looks like that does not work https://github.com/ElementsProject/lightning/actions/runs/3079823060/jobs/4976715878 and my guess is that exporting the requirements.txt is just a monkey conversion of the poetry.lock file, but this does not solve our problem, right? mako is in the poetry.lock file and this failure happen only in the docker, not in the GitHub action, so I think the problem is something else.

This PR is a resurrection of my initial work before my week off, so I put just some more love into it, and also I removed some not used variable configurations, when we remove the autogen file from the repository, this must always be true, otherwise, we can not compile it.

So, the solution that I propose here is to configure better the poetry install and give a configuration that we do not care to install the root package with the command poetry install but just want to resolve the dependencies including the local dependencies

@cdecker
Copy link
Member

cdecker commented Sep 19, 2022

I think you might like #5607, it loosens the requirement in pyln-testing and increases sensitivity to failed commands when setting CI up, so we should actually see the cause (poetry install failing rather than the effects, mako not being present).

@vincenzopalazzo vincenzopalazzo force-pushed the macros/mako_clean branch 2 times, most recently from f73e9d3 to 40def9c Compare September 19, 2022 21:22
@cdecker
Copy link
Member

cdecker commented Sep 22, 2022

I think this is no longer required? Seems #5607 and similar changes did the trick. Turns out it was moslty due to a poetry install that would fail but then it'd continue and fail with a missing mako or mrkd dependency later masking the root cause.

@vincenzopalazzo
Copy link
Collaborator Author

Mh, I think this PR is unrelated to #5607 ? well, I think it is a subset of mine because I had to modify it to test the CI.

I think if we rebase it on the master, we can keep the clean up and the poetry reintroductions, I would like to keep the same build process inside all the CI.

Another think that I like of this PR is that reorganise the poetry configuration, do you think that this is a good addition?

@cdecker
Copy link
Member

cdecker commented Sep 28, 2022

Personally I prefer to keep poetry a dev-only thing, and keep a materialized version of the requirements.txt file in the repo instead. That means we don't end up requiring poetry for builds at all, including CI. mako is a dev-only thing now, i.e., only devs need it when they change one of the files from which dependent files are generated.

We'll eventually migrate over to msggen for all our generation needs, then we can remove mako, but for now I'd rather improve the error message around missing mako than remove the configure option.

The last commit seems fine, though we had a bit of an issue with grpcio 1.48.0, which would misbehave and lock up the entire python process while testing, and it took a long time to figure that one out :-)

@vincenzopalazzo
Copy link
Collaborator Author

Personally I prefer to keep poetry a dev-only thing, and keep a materialized version of the requirements.txt file in the repo instead. That means we don't end up requiring poetry for builds at all, including CI.

Mh @cdecker, I think this is the case of dev-only, in fact this docker is used inside the CI only regarding lnprototest and nothings else, no other user use (must not?) this docker with ubuntu, so why we should remove and keep maintains two different script? this is my only point to keep this PR!

The last commit seems fine, though we had a bit of an issue with grpcio 1.48.0, which would misbehave and lock up the entire python process while testing, and it took a long time to figure that one out :-)

Yeah I think this PR need some rebase on some of the master because this change are made before yours regarding these dependences. If you like to keep this PR I can rebase and prepare for landing.

@vincenzopalazzo
Copy link
Collaborator Author

Lets kik out poetry :)

Source: #5655 (comment)

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.

2 participants