travis: WIP - build and run tests on os: osx

scravy commented at 1:14 pm on July 31, 2018: contributor

This pull requests enables building and running the project with os: osx, sort of natively on os x instead of cross-compiling and not running the tests.

To achieve that the before_install, install, before_script, and script parts were externalized into .travis/before_install etc. These scripts exist for both linux (which runs everything in docker) and osx (which runs everything directly).

Having the steps externalized in individual scripts makes them more readable and subject to shellcheck (lint-shell.sh).

The shellcheck executable segfaults on me, which is why it’s now being run via docker. This has a nice side effect, namely that a more recent version of shellcheck can be used. The runtime of the lint step is not affected to much by it (it’s still around 1 minute as it was before).

The newer shellcheck also comes with more lint rules of which I disabled some as I did not want to touch too much stuff in this single pull request, which is primarily about a reorganization of the travis build.

fanquake added the label Tests on Jul 31, 2018

scravy commented at 1:25 pm on July 31, 2018: contributor

A whitespace slipped in, removed that one (will squash these commits)

scravy force-pushed on Jul 31, 2018

in .travis.yml:129 in aaab3b693d outdated

179+        PACKAGES="python3" DEP_OPTS="NO_WALLET=1"
180         GOAL="install"
181         BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports"
182-# Cross-Mac
183-    - >-
184-        HOST=x86_64-apple-darwin14

MarcoFalke commented at 1:29 pm on July 31, 2018:

I’d prefer if we kept the cross compile on linux, since something similar is used for producing the release binaries.

scravy commented at 3:10 pm on July 31, 2018:

I see. So it should cross-build on linux and for the purpose of this PR do the build on osx additionally, not instead of!

MarcoFalke commented at 4:35 pm on July 31, 2018:

Yes, both would be fine with me.

scravy force-pushed on Jul 31, 2018

ken2812221 commented at 1:47 pm on July 31, 2018: contributor

In my opinion, you could make it run on macos and linux both. Also, please remove the first two commits, e0d9191 and 3144b15.

in test/lint/lint-shell.sh:31 in a1a1e83f02 outdated

26@@ -27,5 +27,8 @@
27 # SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
28 # SC2166: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
29 # SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
30-shellcheck -e SC2001,SC2004,SC2005,SC2006,SC2016,SC2028,SC2046,SC2048,SC2066,SC2086,SC2116,SC2148,SC2162,SC2166,SC2181 \
31-    $(git ls-files -- "*.sh" | grep -vE 'src/(secp256k1|univalue)/')
32+
33+docker run -t -v $(pwd):/code -t koalaman/shellcheck-alpine:v0.5.0 \

promag commented at 2:11 pm on July 31, 2018:

NACK this change. Skip in macos for now?

scravy commented at 3:09 pm on July 31, 2018:

@promag I am unsure about what you mean. This part does not have anything to do with macos. I’d happily factor this one out into a separate pull request.

The lint step is executed in linux/trusty and uses a shellcheck that comes with the distro. It kept segfaulting for me (in travis, not on macos), so I am using a different version which runs in docker and has a different (and more modern) executable in it.

This runs on my mac locally as well as in a linux build in travis equally.

practicalswift commented at 2:38 pm on August 1, 2018:

Please don’t introduce a Docker dependency. If shellcheck does not run on Mac then just skip it.

scravy commented at 2:52 pm on August 1, 2018:

It does run on Mac. I did not introduce this step because it does not run on mac (it does) – as can be seen in #13728 ). For some reason it does segfault though in the os: linux/dist: trusty build.

Here is a build where it segfaults: https://travis-ci.org/scravy/bitcoin/jobs/410232096#L623

scravy commented at 2:55 pm on August 1, 2018:

@practicalswift What is the problem with a docker dependency? To me it seems like the shellcheck binary in ubuntu trusty is broken and segfaults under certain conditions. Here is an issue: https://github.com/koalaman/shellcheck/issues/1053 The docker image pulled in is the official build of shellcheck.

For me to understand: Introducing a dependency for example using brew on osx - is that okay or not?

practicalswift commented at 3:16 pm on August 1, 2018:

@scravy With the suggested change everyone running the linter would effectively be vulnerable if the koalaman Docker Hub account is compromised.

So a clear NACK from me on this specific change. Try to find a more lightweight workaround that doesn’t introduce any additional trust or dependencies.

MarcoFalke commented at 3:20 pm on August 1, 2018:

You could run all the linters in a docker on travis, if that helps?

scravy commented at 10:33 am on August 2, 2018:

@MarcoFalke Running all the linters in a docker container sounds intriguing to me. I might tackle that in a separate pull request. Simply to pull in more recent versions.

promag commented at 12:13 pm on August 2, 2018:

IMO docker should not be a developer dependency and so running linters should also be possible without docker.

promag commented at 2:15 pm on July 31, 2018: member

I also think also building on macos is good. The change is pretty big and so this is going to take a while to be merged. Maybe split in multiple PRs, like Replace travis matrix with jobs?

DrahtBot commented at 3:13 pm on July 31, 2018: member

#13728 (Scripts and tools: Run the CI lint stage on mac by Empact)
#12134 (Build previous releases and run functional tests 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.

in src/chainparams.h:37 in a1a1e83f02 outdated

31@@ -32,9 +32,9 @@ struct CCheckpointData {
32  * See also: CChainParams::TxData, GuessVerificationProgress.
33  */
34 struct ChainTxData {
35-    int64_t nTime;
36-    int64_t nTxCount;
37-    double dTxRate;
38+    int64_t nTime;    //!< UNIX timestamp of last known number of transactions
39+    int64_t nTxCount; //!< total number of transactions between genesis and that timestamp
40+    double dTxRate;   //!< estimated number of transactions per second after that timestamp

MarcoFalke commented at 4:40 pm on July 31, 2018:

Please rebase on master to get rid of these changes.

Sjors commented at 5:56 pm on July 31, 2018: member

Concept ACK, I encounter macOS specific problems quite often.

+1 for keeping the cross-compile version around

There’s also some difference in compiler noisiness between using depends and not using depends, so it might be worth running make for both (but not the full test suite). If you keep (homebrew) system dependencies and Xcode versions up to date we’ll also notice if those start causing problems. Xcode updates have a habit of breaking builds.

scravy force-pushed on Aug 1, 2018

practicalswift commented at 2:39 pm on August 1, 2018: contributor

Concept ACK assuming the Docker dependency added in test/lint/lint-shell.sh is removed :-)

Linting should not require Docker :-)

scravy commented at 2:59 pm on August 1, 2018: contributor

I did remove the docker dependency and am curious on whether the build will fail now and shellcheck segfault.

I seem to not have communicated this properly: The lint step is run using os: linux just as before. shellcheck segfaults for me in travis, not on a mac. There is a PR which makes the lint stage also run on osx ( #13728 ).

Looking at this now I am wondering why the shellcheck segfaults on this branch but does not (also in linux) on that branch.

scravy commented at 3:20 pm on August 1, 2018: contributor

The lint step indeed failed now. I printed uname -a:

016.14s$ test/lint/lint-all.sh
1Linux travis-job-3b3edecb-cf13-406f-8e5d-e8031fd1d16d 4.4.0-101-generic [#124](/bitcoin-bitcoin/124/)~14.04.1-Ubuntu SMP Fri Nov 10 19:05:36 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
2test/lint/lint-shell.sh: line 33:  8542 Segmentation fault      (core dumped) shellcheck -e SC1087,SC1117,SC2001,SC2004,SC2005,SC2006,SC2016,SC2028,SC2046,SC2048,SC2066,SC2086,SC2116,SC2148,SC2162,SC2166,SC2181,SC2206,SC2207,SC2230 $(git ls-files '*.sh' | grep -vE 'src/(secp256k1|univalue)/')
3^---- failure generated from test/lint/lint-shell.sh
4The command "test/lint/lint-all.sh" exited with 1.

I will try to install a more recent version of shellcheck via apt.

practicalswift commented at 3:27 pm on August 1, 2018: contributor

@scravy Are we relying on Travis’ pre-installed shellcheck version? Perhaps the output of these can help:

0$ dpkg -S $(which shellcheck)
1$ dpkg -s shellcheck
2$ shellcheck --version

scravy force-pushed on Aug 1, 2018

MarcoFalke commented at 4:19 pm on August 1, 2018: member

re shellcheck on travis:

If you want to install it with apt, you’d have to set sudo: true, but then you might as well run the linter in some of the minimal docker images?
If you want to install it with the apt extension on sudo: false, it is just as broken as the version that is already installed, so no need to add the extension

Wouldn’t it work if you just left it the way it was run before?

MarcoFalke commented at 4:23 pm on August 1, 2018: member

It seems that you extract the travis.yml into bash scripts. Imo it would help review to split this specific change into a separate commit/pull (for just linux).

in .travis.yml:80 in 397db431f4 outdated

72       python: '3.6'
73       install:
74         - travis_retry pip install flake8
75-        - travis_retry apt-get install shellcheck -y -q
76+        - echo $USER
77+        - travis_retry sudo apt-get install shellcheck -y -q

ken2812221 commented at 7:52 am on August 2, 2018:

This does not require sudo if you use

0addons:
1  apt:
2    packages:
3    - shellcheck

scravy force-pushed on Aug 2, 2018

scravy commented at 9:59 am on August 2, 2018: contributor

@practicalswift @MarcoFalke

My shellcheck woes were because of LC_ALL=C. Apparently it segfaults then. There is a comment in the lint script invoking shellcheck which I overlooked :-( I am now explicitly unseting LC_ALL and it runs as well as before.

in test/lint/lint-shell.sh:33 in 562b81bd3c outdated

28@@ -27,5 +29,7 @@
29 # SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
30 # SC2166: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
31 # SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
32-shellcheck -e SC2001,SC2004,SC2005,SC2006,SC2016,SC2028,SC2046,SC2048,SC2066,SC2086,SC2116,SC2148,SC2162,SC2166,SC2181 \
33-    $(git ls-files -- "*.sh" | grep -vE 'src/(secp256k1|univalue)/')
34+
35+shellcheck -e SC1087,SC1117,SC2001,SC2004,SC2005,SC2006,SC2016,SC2028,SC2046,SC2048,SC2066,SC2086,SC2116,SC2148,SC2162,SC2166,SC2181,SC2206,SC2207,SC2230 \

practicalswift commented at 10:13 am on August 2, 2018:

Please update the comment above (“Disabled warnings”) if you’re intentionally changing the excludes here :-) There should be a 1:1 mapping between the exclude comments and the actual excludes.

Please try to find out if the change in excludes a result of improved shape of the shell scripts (or that shell scripts have been removed?) or if it is due to a change in shellcheck? My guess is that it is due to some “warning prone” shell scripts have been removed from the repo but a confirmation would be nice.

in test/lint/lint-shell.sh:13 in 562b81bd3c outdated

 9@@ -10,6 +10,8 @@
10 # to allow running certain versions of shellcheck that core dump when LC_ALL=C
11 # is set.
12 
13+unset LC_ALL

practicalswift commented at 10:15 am on August 2, 2018:

Can you make this unset conditional on the specific environment where this shellcheck issue occurs? Plus add a comment on why it is done.

scravy commented at 10:29 am on August 2, 2018:

Why? The comment above even says that it specifically does not set LC_ALL because it seg fauls (core cumps) when it is set. Means that if I have LC_ALL set and exported and run this script it will segfault. So to me it looks as if unsetting LC_ALL for good should be required on all platforms.

I can see how this makes one uneasy (at least it makes me uneasy). According to one of the lints all scripts should set LC_ALL=C and export it in order to make it not behave differently on different platforms. Unfortunately that makes shellcheck segfault/coredump. As I learned the hard way.

I am wondering if there is a specific locale which does not make it segfault, but I would assume that that locale is exactly the default locale for the platform. And as long as none of the shell scripts use any fancy characters (basically ASCII only) it should not come to any behavior changes in between any modern platforms.

practicalswift commented at 11:49 am on August 2, 2018:

@scravy I wrote that comment in the code :-) The comment says that LC_ALL=C segfaults under certain shellcheck versions (perhaps a certain Ubuntu distributed shellcheck binary?), so my suggestions is to make the workaround (unset LC_ALL) conditional on where we have found that it is actually needed. That way we document when it is needed which will help us make an informed decision on when to remove the workaround in the future, such as when that specific problematic shellcheck version/binary is no longer in active use. Makes sense?

MarcoFalke commented at 11:54 am on August 2, 2018:

I believe on travis just all versions would segfault :/

practicalswift commented at 12:00 pm on August 2, 2018:

Oh, that’s bad! I didn’t know that. I thought it was generally for a specific Ubuntu version in combination with a specific shellcheck version. No one has been able to reproduce this locally when running same version? @scravy, can you verify that it is not present locally?

scravy commented at 12:26 pm on August 2, 2018:

@practicalswift So I now checked with/and without export LC_ALL=C on Ubuntu 14.04, 16.04, 18.04 in docker and in 18.04 on my ubuntu laptop and the shellcheck version that is part of the distro does not segfault (0.3.3, 0.3.7, 0.4.6) respectively.

Looks to me as if in travis they just have a horrible version.

Okay, so I guess I can make this script use export LC_ALL=C just like the linter actually wants and unset it in case it is running in travis.

practicalswift commented at 12:33 pm on August 2, 2018:

@scravy That would be excellent! :-)

scravy commented at 12:35 pm on August 2, 2018:

@practicalswift @MarcoFalke I extracted this change into: https://github.com/bitcoin/bitcoin/pull/13851

in .travis/script_osx.sh:9 in 562b81bd3c outdated

0@@ -0,0 +1,51 @@
1+#!/usr/bin/env bash
2+#
3+# Copyright (c) 2018 The Bitcoin Core developers
4+# Distributed under the MIT software license, see the accompanying
5+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
6+
7+export LC_ALL=C
8+
9+TRAVIS_COMMIT_LOG=`git log --format=fuller -1`

practicalswift commented at 10:20 am on August 2, 2018:

Nit: Please use new style $(...). Backticks are discouraged nowadays. See shellcheck documentation. Same comment applies to all usages of backticks.

Also, try running shellcheck without any excluded warnings on your newly introduced shell scripts to make sure they pass without warnings.

scravy commented at 10:40 am on August 2, 2018: contributor

I started splitting this PR up into smaller ones. Here is the first part as proposed by @Empact to have the matrix/jobs transition in a separate PR: https://github.com/bitcoin/bitcoin/pull/13849

DrahtBot added the label Needs rebase on Aug 2, 2018

MarcoFalke referenced this in commit 9c4324d866 on Aug 2, 2018

promag commented at 11:28 am on August 3, 2018: member

I started splitting this PR up into smaller ones. Here is the first part as proposed by @Empact to have the matrix/jobs transition in a separate PR: #13849

👀

MarcoFalke referenced this in commit 2b67354aa5 on Aug 3, 2018

scravy commented at 2:01 pm on August 3, 2018: contributor

I extracted the move-script-sections-to-files-part into #13863 as suggested by @MarcoFalke

Once that one is reviewed I will rebase this one which should boil down to a really tiny change then.

MarcoFalke referenced this in commit ca4510c15d on Aug 27, 2018

scravy force-pushed on Aug 27, 2018

DrahtBot removed the label Needs rebase on Aug 27, 2018

scravy force-pushed on Aug 27, 2018

scravy force-pushed on Aug 28, 2018

scravy renamed this:
~~travis: build and run tests on os: osx~~
travis: WIP - build and run tests on os: osx
on Aug 28, 2018

scravy force-pushed on Aug 28, 2018

run tests on native osx build 9accf543f4

scravy force-pushed on Aug 28, 2018

in .travis/test_06_script.sh:86 in 9accf543f4

81+cd build || exit 1
82+END_FOLD
83+
84+echo "configure"
85+BEGIN_FOLD configure
86+../configure --cache-file=config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( cat config.log && false)

practicalswift commented at 2:15 pm on September 11, 2018:

~~Double quote to prevent globbing and word splitting. Same below. Fix throughout :-)~~

~~Verify with shellcheck .travis/test_06_script.sh | grep SC2086.~~

ken2812221 commented at 9:15 am on September 12, 2018:

I think the config arguments should be split off. No?

practicalswift commented at 9:48 pm on September 25, 2018:

Oh, sorry. Was a bit too quick with that comment :-)

MarcoFalke commented at 3:40 pm on October 26, 2018: member

Should be marked “up for grabs”?

fanquake added the label Up for grabs on Oct 28, 2018

fanquake closed this on Oct 28, 2018

PastaPastaPasta referenced this in commit e48dd50d6e on Jul 29, 2020

PastaPastaPasta referenced this in commit 6e35655b01 on Jul 29, 2020

PastaPastaPasta referenced this in commit a859ea12a9 on Jul 29, 2020

fanquake removed the label Up for grabs on Mar 2, 2021

fanquake commented at 8:10 am on March 2, 2021: member

Removing “Up for grabs” as we have left Travis and have a “native” macOS build via Cirrus.

Munkybooty referenced this in commit e1e77958ce on Jun 30, 2021

Munkybooty referenced this in commit 17362b8fd4 on Jul 1, 2021

Munkybooty referenced this in commit a3a381b28b on Jul 1, 2021

Munkybooty referenced this in commit a872c78836 on Jul 2, 2021

Munkybooty referenced this in commit 251ce629e7 on Jul 2, 2021

Munkybooty referenced this in commit a33cfb7c92 on Jul 4, 2021

UdjinM6 referenced this in commit 50d8d8023e on Jul 7, 2021

Munkybooty referenced this in commit 9d9efce3f4 on Jul 7, 2021

UdjinM6 referenced this in commit 77bbcd9612 on Jul 7, 2021

Munkybooty referenced this in commit 1ba72e6323 on Jul 7, 2021

Munkybooty referenced this in commit 4c2211dd41 on Jul 8, 2021

Munkybooty referenced this in commit 8e25e2f0fe on Jul 8, 2021

vijaydasmp referenced this in commit 976104fe1a on Sep 30, 2021

vijaydasmp referenced this in commit 1c965b3905 on Oct 1, 2021

gades referenced this in commit d659ef0fb4 on Apr 20, 2022

DrahtBot locked this on Aug 16, 2022

travis: WIP - build and run tests on os: osx #13816