Skip to content

ci: Build python from source in "lint" task #26716

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.

Sign up for GitHub

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
Merged
maflcko merged 2 commits into from
Jan 17, 2023
Merged

ci: Build python from source in "lint" task #26716

maflcko merged 2 commits into from
Jan 17, 2023

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Dec 17, 2022 •
edited
Loading

This PR:

Key advantages of this PR over others:

  • it uses pyenv's python-build standalone
  • requires no additional computational resources

Note for testing. The lint task must success regardless of whether the python_cache is populated or invalidated.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 17, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake, MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot added the Tests label Dec 17, 2022
@hebasto hebasto mentioned this pull request Dec 17, 2022
Closed
@fanquake
Copy link
Member

fanquake commented Dec 17, 2022

it uses pyenv's python-build standalone; no other tools required

As far as I can tell, pyenv install is just a wrapper around python-build. You still need the same tools (compiler / linker / all the python build dependencies) to compile Python regardless of how you compile it. I guess the difference here is just not fully installing pyenv?

requires no additional computational resources

GIven that pyenv install and python-build are the same thing, why is there a difference in computational resources required?

maflcko
maflcko reviewed Dec 17, 2022
View reviewed changes
@hebasto
Copy link
Member Author

hebasto commented Dec 17, 2022

it uses pyenv's python-build standalone; no other tools required

As far as I can tell, pyenv install is just a wrapper around python-build. You still need the same tools (compiler / linker / all the python build dependencies) to compile Python regardless of how you compile it. I guess the difference here is just not fully installing pyenv?

Correct. Dropped "no other tools required" from the PR description.

requires no additional computational resources

GIven that pyenv install and python-build are the same thing, why is there a difference in computational resources required?

I reckon the lower memory usage is due to using clang instead of gcc.

@maflcko maflcko mentioned this pull request Jan 9, 2023
Merged
maflcko
maflcko reviewed Jan 9, 2023
View reviewed changes
@hebasto
Copy link
Member Author

hebasto commented Jan 14, 2023

Updated 3f602a6 -> f99260f (pr26716.02 -> pr26716.03, diff):

maflcko
maflcko reviewed Jan 16, 2023
View reviewed changes
@hebasto hebasto force-pushed the 221217-pyenv branch 2 times, most recently from e9c8538 to d418803 Compare January 16, 2023 10:57
@hebasto
Copy link
Member Author

hebasto commented Jan 16, 2023 •
edited
Loading

Updated f99260f -> d418803 (pr26716.03 -> pr26716.05, diff):

@hebasto
Copy link
Member Author

hebasto commented Jan 16, 2023

CI tasks with the python cache being:

fanquake
fanquake reviewed Jan 16, 2023
View reviewed changes
@hebasto
Copy link
Member Author

hebasto commented Jan 16, 2023

Updated d418803 -> 123043e (pr26716.05 -> pr26716.06, diff):

fanquake
fanquake approved these changes Jan 17, 2023
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 123043e

@maflcko
Copy link
Member

maflcko commented Jan 17, 2023

ACK 123043e

maflcko
maflcko approved these changes Jan 17, 2023
View reviewed changes
ci/lint/04_install.sh
${CI_RETRY_EXE} apt-get update
${CI_RETRY_EXE} apt-get install -y --reinstall git
)
${CI_RETRY_EXE} apt-get install -y curl git gawk jq xz-utils
Copy link
Member

@maflcko maflcko Jan 17, 2023

Choose a reason for hiding this comment

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

Why add xz-utils here when it is only needed when compiling python?

Copy link
Member Author

@hebasto hebasto Jan 17, 2023

Choose a reason for hiding this comment

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

It is required here:

bitcoin/ci/lint/04_install.sh

Line 37 in 01ec530

curl -sL "https://github.com/koalaman/shellcheck/releases/download/${SHELLCHECK_VERSION}/shellcheck-${SHELLCHECK_VERSION}.linux.x86_64.tar.xz" | tar --xz -xf - --directory /tmp/

@maflcko maflcko merged commit 01ec530 into bitcoin:master Jan 17, 2023
@hebasto hebasto deleted the 221217-pyenv branch January 17, 2023 17:55
@hebasto hebasto mentioned this pull request Jan 17, 2023
Merged
@jamesob
Copy link
Contributor

jamesob commented Jan 17, 2023

This change points to the wastefulness of our current CI setup as alluded to in the Cirrus docs:

image

On every CI run, we're doing the same apt installs and Python setup. We should probably just specify a Dockerfile, the resulting image of which can be cached on the Cirrus side. I think that would also simplify CI on our end and make it easier to run locally.

If there's interest I can create an issue for this.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 17, 2023
Merge bitcoin#26716: ci: Build python from source in "lint" task
c499032 
123043e ci: Bump lint task image to Ubuntu Jammy (Hennadii Stepanov)
9b86114 ci: Use pyenv's `python-build` to install Python in lint task (Hennadii Stepanov)

Pull request description:

  This PR:
  - is an alternative to bitcoin#26581 and bitcoin#26637
  - closes bitcoin#26548

  Key advantages of this PR over others:
  - it uses pyenv's `python-build` [standalone](https://github.com/pyenv/pyenv/tree/master/plugins/python-build#using-python-build-standalone)
  - requires no additional computational resources

  Note for testing. The lint task must success regardless of whether the `python_cache` is populated or invalidated.

ACKs for top commit:
  MarcoFalke:
    ACK 123043e
  fanquake:
    ACK 123043e

Tree-SHA512: ba0fcdd4f2939a59692b173dcd1f5704444cfcfbb8111538c6f8160056d0536bba250e4f9b0f8c66f8b454e52034bf36ffe6afae76cdc0f7cc5b58b576d790ba
@maflcko
Copy link
Member

maflcko commented Jan 17, 2023

Yeah, I did this because I want to be able to run the stuff easily locally: https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally

I don't know enough about docker to figure out if there is a more efficient but equally simply way to do this.

Overall I am thinking that it shouldn't matter if we download the packages or download a pod layer with the packages installed. (Modulo a slow mirror, which should ideally be solved by using a fast mirror, but there doesn't seem to be an easy way to do this? Also modulo the time it takes to run the install script)

@bitcoin bitcoin locked and limited conversation to collaborators Jan 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@fanquake fanquake fanquake approved these changes

+1 more reviewer

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ci: Use pyenv for the lint task

5 participants

@hebasto @DrahtBot @fanquake @maflcko @jamesob