ci: Move linter task to cirrus #20658

dhruv commented at 1:07 AM on December 15, 2020: member

Solves #20467: Move linter to Cirrus-CI as Travis-CI.org is shutting down

fanquake added the label Tests on Dec 15, 2020

in ci/lint/lint_run_all.sh:13 in 52b724efb3 outdated

   8 | +
   9 | +set -o errexit; source ./ci/test/00_setup_env.sh
  10 | +set -o errexit; source ./ci/test/03_before_install.sh
  11 | +set -o errexit; source ./ci/lint/04_install.sh
  12 | +set -o errexit; source ./ci/lint/05_before_script.sh
  13 | +set -o errexit; source ./ci/lint/06_script.sh

MarcoFalke commented at 7:01 AM on December 15, 2020:

The linter scripts heavily depend on the ci platform because they use $TRAVIS or $CIRRUS env vars, so I think the script here can just be inlined into the platform.yml

in ci/lint/05_before_script.sh:9 in 52b724efb3 outdated

   5 | @@ -6,4 +6,4 @@
   6 |  
   7 |  export LC_ALL=C
   8 |  
   9 | -git fetch --unshallow
  10 | +git fetch

MarcoFalke commented at 7:02 AM on December 15, 2020:

why?

dhruv commented at 5:36 PM on December 15, 2020:

On Cirrus-CI, I received:

fatal: --unshallow on a complete repository does not make sense

in ci/lint/04_install.sh:16 in 52b724efb3 outdated

  18 | -travis_retry pip3 install yq
  19 | -travis_retry pip3 install mypy==0.781
  20 | +pip3 install codespell==1.17.1
  21 | +pip3 install flake8==3.8.3
  22 | +pip3 install yq
  23 | +pip3 install mypy==0.781

MarcoFalke commented at 7:03 AM on December 15, 2020:

there is ./ci/retry, which can be used instead

in .cirrus.yml:61 in 52b724efb3 outdated

  53 | @@ -54,6 +54,15 @@ global_task_template: &GLOBAL_TASK_TEMPLATE
  54 |  #  install_script:
  55 |  #    - choco install python --version=3.7.7 -y
  56 |  
  57 | +lint_task:
  58 | +  name: 'lint'
  59 | +  skip: $CIRRUS_REPO_FULL_NAME == "bitcoin-core/gui" && $CIRRUS_PR == ""  # No need to run on the read-only mirror, unless it is a PR.
  60 | +  container:
  61 | +    image: ubuntu:bionic

MarcoFalke commented at 7:05 AM on December 15, 2020:

Please re-add the comment here that says

    For   python: '3.6' # Oldest supported version according to doc/dependencies.md

in .cirrus.yml:63 in 52b724efb3 outdated

  58 | +  name: 'lint'
  59 | +  skip: $CIRRUS_REPO_FULL_NAME == "bitcoin-core/gui" && $CIRRUS_PR == ""  # No need to run on the read-only mirror, unless it is a PR.
  60 | +  container:
  61 | +    image: ubuntu:bionic
  62 | +  pip_cache:
  63 | +    folder: /tmp/pip_cache_dir

MarcoFalke commented at 7:07 AM on December 15, 2020:

Is this really needed? I somehow supspect that setting up the cache and fetching from it is slower than fetching from the pip servers directly. Or at the least, it is not measurable and we don't care about a few milliseconds here or there anyway.

in .cirrus.yml:63 in 52b724efb3 outdated

  53 | @@ -54,6 +54,15 @@ global_task_template: &GLOBAL_TASK_TEMPLATE
  54 |  #  install_script:
  55 |  #    - choco install python --version=3.7.7 -y
  56 |  
  57 | +lint_task:
  58 | +  name: 'lint'
  59 | +  skip: $CIRRUS_REPO_FULL_NAME == "bitcoin-core/gui" && $CIRRUS_PR == ""  # No need to run on the read-only mirror, unless it is a PR.

MarcoFalke commented at 7:08 AM on December 15, 2020:

Maybe move this to a GLOBAL_BASE_TEMPLATE to avoid duplication?

MarcoFalke approved

MarcoFalke commented at 7:08 AM on December 15, 2020: member

ACK, just some nits

in .cirrus.yml:64 in 52b724efb3 outdated

  59 | +  skip: $CIRRUS_REPO_FULL_NAME == "bitcoin-core/gui" && $CIRRUS_PR == ""  # No need to run on the read-only mirror, unless it is a PR.
  60 | +  container:
  61 | +    image: ubuntu:bionic
  62 | +  pip_cache:
  63 | +    folder: /tmp/pip_cache_dir
  64 | +  script: set -o errexit; source ./ci/lint/lint_run_all.sh

MarcoFalke commented at 7:10 AM on December 15, 2020:

Could run the linters with compute credits? The most frustrating thing is waiting for the linters to fail (or pass)

  # For faster CI feedback, immediately schedule the linters. https://cirrus-ci.org/pricing/#compute-credits
  use_compute_credits: $CIRRUS_REPO_FULL_NAME == 'bitcoin/bitcoin'

DrahtBot commented at 7:40 AM on December 15, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19952 (build, ci: Add file-based logging for individual packages by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

dhruv force-pushed on Dec 15, 2020

dhruv commented at 5:37 PM on December 15, 2020: member

Comments addressed. Ready for re-review.

dhruv force-pushed on Dec 15, 2020

in .cirrus.yml:21 in 38baea179a outdated

  14 | @@ -15,11 +15,14 @@ env:
  15 |    CCACHE_SIZE: "200M"
  16 |    CCACHE_DIR: "/tmp/ccache_dir"
  17 |  
  18 | -### Global task template
  19 | -
  20 | +### Base template
  21 |  # https://cirrus-ci.org/guide/tips-and-tricks/#sharing-configuration-between-tasks
  22 | +base_template: &BASE_TEMPLATE
  23 | +  skip: $CIRRUS_REPO_FULL_NAME == "bitcoin-core/gui" && $CIRRUS_PR == "" # No need to run on the read-only mirror, unless it is a PR. https://cirrus-ci.org/guide/writing-tasks/#conditional-task-execution

MarcoFalke commented at 5:49 PM on December 15, 2020:

merge_base_script should also be part of the base template

in .cirrus.yml:60 in 38baea179a outdated

  56 | @@ -54,6 +57,20 @@ global_task_template: &GLOBAL_TASK_TEMPLATE
  57 |  #  install_script:
  58 |  #    - choco install python --version=3.7.7 -y
  59 |  
  60 | +lint_task:

MarcoFalke commented at 5:50 PM on December 15, 2020:

task:

Can be shorter (because you provided a name: )

in .cirrus.yml:64 in 38baea179a outdated

  56 | @@ -54,6 +57,20 @@ global_task_template: &GLOBAL_TASK_TEMPLATE
  57 |  #  install_script:
  58 |  #    - choco install python --version=3.7.7 -y
  59 |  
  60 | +lint_task:
  61 | +  name: 'lint'
  62 | +  << : *BASE_TEMPLATE
  63 | +  container:
  64 | +    image: ubuntu:bionic # For python 3.6, oldest supported version according to doc/dependencies.md

MarcoFalke commented at 5:51 PM on December 15, 2020:

Can reduce the cpu to 1 to cut the bill in half?

dhruv commented at 7:09 PM on December 15, 2020:

container:
   image: ubuntu:bionic # For python 3.6, oldest supported version according to doc/dependencies.md
   cpu: 1.0 # Cut the bill in half

Resulted in:

Failed to start an instance: INVALID_ARGUMENT: Bad Request.

Could you share a pointer on how to do this? I found nothing except this in cirrus docs.

MarcoFalke commented at 6:24 AM on December 16, 2020:

cpu can only be integral, not floating point.

Also, you might have to move this hunk into the GLOBAL_TASK_TEMPLATE to avoid collisions:

timeout_in: 120m  # https://cirrus-ci.org/faq/#instance-timed-out
container:
  # https://cirrus-ci.org/faq/#are-there-any-limits
  # Each project has 16 CPU in total, assign 2 to each container, so that 8 tasks run in parallel
  cpu: 2
  memory: 8G  # Set to 8GB to avoid OOM. https://cirrus-ci.org/guide/linux/#linux-containers
  kvm: true  # Use kvm to avoid spurious CI failures in the default virtualization cluster, see [#20093](/bitcoin-bitcoin/20093/)

MarcoFalke commented at 6:28 AM on December 16, 2020:

You can also leave it as is for now. Can be addressed later (or in a new commit)

dhruv commented at 1:55 AM on December 17, 2020:

That seems to have done the trick. I've added a commit: 4045a67.

in .cirrus.yml:70 in 38baea179a outdated

  64 | +    image: ubuntu:bionic # For python 3.6, oldest supported version according to doc/dependencies.md
  65 | +  # For faster CI feedback, immediately schedule the linters. https://cirrus-ci.org/pricing/#compute-credits
  66 | +  use_compute_credits: $CIRRUS_REPO_FULL_NAME == 'bitcoin/bitcoin'
  67 | +  lint_script:
  68 | +    - set -o errexit; source ./ci/test/00_setup_env.sh
  69 | +    - set -o errexit; source ./ci/test/03_before_install.sh

MarcoFalke commented at 5:52 PM on December 15, 2020:

Can each line be a separate script?

in ci/lint/06_script.sh:30 in 38baea179a outdated

  24 | @@ -25,8 +25,8 @@ test/lint/check-doc.py
  25 |  test/lint/check-rpc-mappings.py .
  26 |  test/lint/lint-all.sh
  27 |  
  28 | -if [ "$TRAVIS_REPO_SLUG" = "bitcoin/bitcoin" ] && [ "$TRAVIS_EVENT_TYPE" = "cron" ]; then
  29 | +if [ "$CIRRUS_REPO_FULL_NAME" = "bitcoin/bitcoin" ] && [ -n "$CIRRUS_CRON" ]; then
  30 |      git log --merges --before="2 days ago" -1 --format='%H' > ./contrib/verify-commits/trusted-sha512-root-commit
  31 | -    travis_retry gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys $(<contrib/verify-commits/trusted-keys) &&
  32 | +    gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys $(<contrib/verify-commits/trusted-keys) &&

MarcoFalke commented at 5:56 PM on December 15, 2020:

the retry missing here?

MarcoFalke approved

MarcoFalke commented at 5:56 PM on December 15, 2020: member

re-ACK

ci: Move linter task to cirrus 739d39022d

dhruv force-pushed on Dec 15, 2020

hebasto commented at 6:18 PM on December 15, 2020: member

Concept ACK.

dhruv commented at 7:11 PM on December 15, 2020: member

Comments addressed. Ready for review.

MarcoFalke commented at 6:26 AM on December 16, 2020: member

ACK 739d39022d2959c1114c14a0065daebf4fe812c1

ci: Use cpu=1 for linter 4045a6722c

dhruv commented at 6:29 AM on December 17, 2020: member

Ready for re-review.

in .cirrus.yml:65 in 4045a6722c

  61 | @@ -62,6 +62,7 @@ task:
  62 |    << : *BASE_TEMPLATE
  63 |    container:
  64 |      image: ubuntu:bionic # For python 3.6, oldest supported version according to doc/dependencies.md
  65 | +    cpu: 1 # Cut bill in half for linting

MarcoFalke commented at 7:46 AM on December 17, 2020:

No need for that comment. Can remove or just say 1 is sufficient

    cpu: 1

    cpu: 1  # No need for more CPUs

MarcoFalke commented at 7:47 AM on December 17, 2020:

btw, based on this plot https://cirrus-ci.com/task/4624095184683008

memory can also be set to 1

MarcoFalke commented at 8:04 AM on December 17, 2020: member

ACK 4045a6722c884be779e86016313061ac6ff80808

MarcoFalke merged this on Dec 17, 2020

MarcoFalke closed this on Dec 17, 2020

hebasto commented at 1:41 PM on December 17, 2020: member

On my personal account having some "fatal: Not a valid object name master" warnings:

hebasto commented at 1:57 PM on December 17, 2020: member

Also in the merge commit, https://cirrus-ci.com/task/6086189209878528?command=lint#L50:

Skipping shell linting since shellcheck is not installed.

laanwj deleted a comment on Dec 17, 2020

MarcoFalke commented at 2:17 PM on December 17, 2020: member

Missing packages are installed in #20682

dhruv commented at 3:14 PM on December 17, 2020: member

Thanks for catching those, @hebasto. I should've looked closer.

sidhujag referenced this in commit a0bc41e79f on Dec 17, 2020

sidhujag referenced this in commit 346d54527d on Dec 17, 2020

MarcoFalke referenced this in commit f061da2887 on Dec 21, 2020

sidhujag referenced this in commit 885721f23e on Dec 21, 2020

DrahtBot locked this on Feb 15, 2022