ci: Disable macOS functional tests on forked repos to avoid timeouts #19495

hebasto · 2020-07-11T17:42:17Z

See: bitcoin-core/gui#5 (comment)

Additionally, this PR:

updates macOS image to the recent 10.15.5 version
drops Homebrew caching as the Travis Homebrew addon have been used since ci: Use Homebrew addon on native macOS #18438

My forked repo build: https://travis-ci.org/github/hebasto/bitcoin/jobs/707200431

DrahtBot · 2020-07-11T19:25:03Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: Add NAT-PMP port forwarding support #18077 (net: Add NAT-PMP port forwarding support by hebasto)
util: add RunCommandParseJSON #15382 (util: add RunCommandParseJSON by Sjors)

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.

promag · 2020-07-12T01:06:37Z

I think commit "ci: Update macOS image to 10.15.5 on Travis" doesn't belong here. What is the motivation? ACK remaining commits.

hebasto · 2020-07-12T08:45:29Z

@promag

I think commit "ci: Update macOS image to 10.15.5 on Travis" doesn't belong here. What is the motivation? ACK remaining commits.

The mentioned commit has been dropped.

promag · 2020-07-12T08:49:39Z

ACK 0b9e1e6

maflcko · 2020-07-12T09:28:55Z

.travis.yml

  - export CONTINUE=1
  - if [ $SECONDS -gt 1200 ]; then export CONTINUE=0; fi  # Likely the depends build took very long
  - if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi  # continue on repos with extended build time (90 minutes)
  - if [ $CONTINUE = "1" ]; then set -o errexit; source ./ci/test/06_script_a.sh; else set +o errexit; echo "$CACHE_ERR_MSG"; false; fi
-  - if [ $SECONDS -gt 2000 ]; then export CONTINUE=0; fi  # Likely the build took very long; The tests take about 1000s, so we should abort if we have less than 50*60-1000=2000s left
+  - if [ $SECONDS -gt 2000 && $LONG_RUNNING_TEST = "1" ]; then export CONTINUE=0; fi  # Likely the build took very long; The tests take about 1000s, so we should abort if we have less than 50*60-1000=2000s left


Should this be an && or || and why? Also, why use the runtime heuristic in combination with the heuristic in line 46. Wouldn't it be easier to simply export LONG_RUNNING_TEST for the configs that need it? I.e. right in the line where the other env vars are defined:

env: >- FILE_ENV="..." LONG_...

^ this will not work for forked repos as the LONG_RUNNING_TEST actually should depend on the TRAVIS_REPO_SLUG.

Another suggestion is to export LONG_RUNNING_TEST="true" by default in 00_setup_env.sh and set it in a job setup script 00_setup_env_*.sh:

if [ "$TRAVIS_REPO_SLUG" != "bitcoin/bitcoin" ]; then export RUN_FUNCTIONAL_TESTS="false" export LONG_RUNNING_TEST="false" fi

If you don't set LONG_RUNNING_TEST to anything for the macos config, everything will work fine in all repos, no?

bitcoin/bitcoin will run the functional tests, and forked repos won't. Independent of LONG_RUNNING_TEST

If you don't set LONG_RUNNING_TEST to anything for the macos config, everything will work fine in all repos, no?

bitcoin/bitcoin will run the functional tests, and forked repos won't. Independent of LONG_RUNNING_TEST

With cleared (or failed) cache forked repos could experience timeouts (native macos now, but any build could fail with timeout in the future).

The mean of LONG_RUNNING_TEST is:

it is set -- do not continue tests if not enough time remains. This should be done to avoid timeouts in forked repos

it is unset -- do not take into account the remaining time, just continue. This should be done in cases of manual disabled tests in forked repos.

Actually, the LONG_RUNNING_TEST only affects forked repos due to the following code:

bitcoin/.travis.yml

Lines 51 to 52 in 4db44ac

- if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi # continue on repos with extended build time (90 minutes)

- if [ $CONTINUE = "1" ]; then set -o errexit; source ./ci/test/06_script_b.sh; else set +o errexit; echo "$CACHE_ERR_MSG"; false; fi

Ok, so this seems to affect more than just macos. Hmm. I am not sure if it is expected to fork the repo, run the tests on a commit and then see a green checkmark when the functional tests haven't even been run. Then the commit is proposed to bitcoin/bitcoin and the tests fail.

Excluding macOS is fine because I am not aware this ever detected a bug that was not detected by any of the other builds. Though, disabling all tests for forked repos seems a bit too much, no?

If travis can't give us what we need, we should switch providers instead of disabling tests conditionally.

maflcko · 2020-07-12T09:29:43Z

.travis.yml

@@ -43,11 +41,12 @@ before_script:
  - for i in {1..4}; do echo "$(sleep 500)" ; done &
  - set -o errexit; source ./ci/test/05_before_script.sh &> "/dev/null"
 script:
+  - if [[ -n "$USE_VALGRIND" || "$RUN_FUNCTIONAL_TESTS" = "true" || "$RUN_FUZZ_TESTS" = "true" ]]; then export LONG_RUNNING_TEST=1; fi


are the fuzz tests timing out as well these days?

I hope not! Do we have any evidence suggesting that? If so I'll investigate how we could speed things up.

maflcko

ACK

maflcko

withdraw ACK for now

maflcko

withdraw ACK for now

hebasto · 2020-07-13T09:22:04Z

Updated 0b9e1e6 -> 12f642d (pr19495.02 -> pr19495.03, diff):

timeout changes affect only native macOS build now
added "ci: Fix configure options for macOS builds" commit

--

However, still working on performance improving for the native macOS build on Travis.

maflcko

ACK

maflcko · 2020-07-13T09:44:40Z

.travis.yml

@@ -48,7 +46,7 @@ script:
  - if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi  # continue on repos with extended build time (90 minutes)
  - if [ $CONTINUE = "1" ]; then set -o errexit; source ./ci/test/06_script_a.sh; else set +o errexit; echo "$CACHE_ERR_MSG"; false; fi
  - if [ $SECONDS -gt 2000 ]; then export CONTINUE=0; fi  # Likely the build took very long; The tests take about 1000s, so we should abort if we have less than 50*60-1000=2000s left
-  - if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi  # continue on repos with extended build time (90 minutes)
+  - if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" || $CONTINUE_IN_FORKED_REPO = "true" ]; then export CONTINUE=1; fi  # continue on repos with extended build time (90 minutes)


Suggested change

- if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" || $CONTINUE_IN_FORKED_REPO = "true" ]; then export CONTINUE=1; fi # continue on repos with extended build time (90 minutes)

- if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi # continue on repos with extended build time (90 minutes)

Is this needed? With a warm cache, this should already continue, and without a warm cache it might or might not continue.

I think we should either keep track of when to continue manually for all configs or with the time-heuristic for all configs, not a mix of both.

It is sufficient to specify CCACHE_DIR to cache. Also this change fixes ccache on native macOS build.

hebasto · 2020-07-13T23:45:48Z

Updated 12f642d -> 60824b3 (pr19495.03 -> pr19495.04):

addressed @MarcoFalke's comment

I think we should either keep track of when to continue manually for all configs or with the time-heuristic for all configs, not a mix of both.

The time-heuristic is used for all configs. It could customized per build with the EXPECTED_TESTS_DURATION_IN_SECONDS environment variable.

added "ci: Do not activate Travis ccache caching strategy" commit that fixes ccache for native macOS build on Travis

The 300M value of the CCACHE_SIZE variable was chosen after dozens of tests as an optimal one.

…os to avoid timeouts 60824b3 ci: Fix configure options for macOS builds (Hennadii Stepanov) 687939e ci: Drop Homebrew caching while using Homebrew addon on Travis (Hennadii Stepanov) 557d3f1 ci: Do not activate Travis ccache caching strategy (Hennadii Stepanov) 2d74742 ci: Disable functional tests on forked repos to avoid timeouts for macOS (Hennadii Stepanov) Pull request description: See: bitcoin-core/gui#5 (comment) Additionally, this PR: - updates macOS image to the recent 10.15.5 version - drops Homebrew caching as the Travis Homebrew addon have been used since bitcoin#18438 My forked repo build: https://travis-ci.org/github/hebasto/bitcoin/jobs/707200431 Top commit has no ACKs. Tree-SHA512: 398e935f965a04babeb10e7b26d2341562f21a1ef671c2e7cc97c9ec79d5c31643f81ca18561ab7714b5c52e19df2e4bffe4223eadbab984daa9418ffbf8c2a8

DrahtBot added the Tests label Jul 11, 2020

DrahtBot mentioned this pull request Jul 11, 2020

net: Add NAT-PMP port forwarding support #18077

Merged

hebasto force-pushed the 200711-ci-mac branch from 7ecf08e to 0b9e1e6 Compare July 12, 2020 08:44

maflcko changed the title ~~ci: Disable functional tests on forked repos to avoid timeouts for macOS~~ ci: Disable macOS functional tests on forked repos to avoid timeouts Jul 12, 2020

maflcko reviewed Jul 12, 2020

View reviewed changes

maflcko approved these changes Jul 12, 2020

View reviewed changes

maflcko reviewed Jul 13, 2020

View reviewed changes

maflcko suggested changes Jul 13, 2020

View reviewed changes

hebasto force-pushed the 200711-ci-mac branch from 0b9e1e6 to 12f642d Compare July 13, 2020 09:12

maflcko reviewed Jul 13, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 13, 2020

util: add RunCommandParseJSON #15382

Merged

1 task

hebasto added 4 commits July 14, 2020 02:34

ci: Disable functional tests on forked repos to avoid timeouts for macOS

2d74742

ci: Do not activate Travis ccache caching strategy

557d3f1

It is sufficient to specify CCACHE_DIR to cache. Also this change fixes ccache on native macOS build.

ci: Drop Homebrew caching while using Homebrew addon on Travis

687939e

ci: Fix configure options for macOS builds

60824b3

hebasto force-pushed the 200711-ci-mac branch from 12f642d to 60824b3 Compare July 13, 2020 23:39

maflcko merged commit 90b5fc9 into bitcoin:master Jul 14, 2020

hebasto deleted the 200711-ci-mac branch July 14, 2020 07:36

hebasto mentioned this pull request Jul 14, 2020

[meta] GUI repo Questions and Feedback bitcoin-core/gui#5

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	- if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi # continue on repos with extended build time (90 minutes)
	- if [ $CONTINUE = "1" ]; then set -o errexit; source ./ci/test/06_script_b.sh; else set +o errexit; echo "$CACHE_ERR_MSG"; false; fi

ci: Disable macOS functional tests on forked repos to avoid timeouts #19495

ci: Disable macOS functional tests on forked repos to avoid timeouts #19495

Uh oh!

Conversation

hebasto commented Jul 11, 2020

Uh oh!

DrahtBot commented Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Jul 12, 2020

Uh oh!

hebasto commented Jul 12, 2020

Uh oh!

promag commented Jul 12, 2020

Uh oh!

maflcko Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

practicalswift Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 13, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jul 11, 2020 •

edited

Loading

hebasto Jul 12, 2020 •

edited

Loading

hebasto Jul 12, 2020 •

edited

Loading

hebasto commented Jul 13, 2020 •

edited

Loading