Move build system over to custom runner #1227

DRMacIver · 2018-04-12T15:28:51Z

This is a start of trying to unify our build system a bit more so that it's less spread over Python/make/shell/etc. There's still a bit of shell (both for bootstrapping into Python and some of our test scripts remain).

The main goal in doing this is that it will make it easier to refactor our decisions about what should be running. Right now this is mostly feature parity with a few things that got fixed while I was in the area (e.g. we now check a few more RST files).

I'm not super happy with this code, but it's a decent starting point.

alexwlchan · 2018-04-12T15:35:38Z

build.sh

+#!/usr/bin/env bash
+
+# This script is here to bootstrap the Hypothesis build process into a working
+# version of Python, then hand over to the actual Hypothesi build runner (which


nit: Hypothesi

alexwlchan · 2018-04-12T15:36:23Z

build.sh

+# version of Python, then hand over to the actual Hypothesi build runner (which
+# is written in Python instead of bash).
+
+set -e -u -x


minor: I prefer the verbose forms (e.g. set -o errexit) for readability.

alexwlchan

Oooooooh. I don’t have time to review this right now, hopefully soon!

DRMacIver · 2018-04-12T20:05:07Z

It looks like the remaining problems are about how tox looks up interpreters. I'm not going to fix them tonight.

I'd been eyeing up killing tox as part of this and I think that may have tipped me over towards doing that.

Zac-HD · 2018-04-13T08:36:16Z

Makefile

@@ -0,0 +1,6 @@
+# You don't need to use this Makefile and should use build.sh instead. This is
+# just here so that us poor souls who remember the Make based system and keep
+# typing "make target" can ease our transition to the new sytsem.


sytsem -> system

(you poor soul 😉)

alexwlchan · 2018-04-15T15:28:50Z

hypothesis-python/scripts/basic-test.sh

@@ -1,7 +1,7 @@
 #!/bin/bash
 set -e -o xtrace

-cd "$(dirname "$0")"/../hypothesis-python
+cd "$(dirname "$0")"/..


Is this meant to be cd'ing to the directory this script lives in?

If so, would https://stackoverflow.com/a/246128/1558022 potentially be more robust? (e.g. if this script is called from something else, is $0 set correctly?)

@DRMacIver the 👍 makes me think you wanted to take this suggestion, but it looks like you didn't?

(I don't have any opinion and definitely haven't read that SO answer. Just checking in to see if there's something you wanted to do and didn't here.)

Whoops! I did it in a bunch of places but not the one that @alexwlchan actually commented on. Thanks.

alexwlchan · 2018-04-15T15:29:30Z

hypothesis-python/tox.ini

@@ -146,7 +146,7 @@ commands =
 python -m coverage debug sys
 python -m coverage run --rcfile=.coveragerc -m pytest -n0 --strict tests/cover tests/datetime tests/py3 tests/numpy tests/pandas --maxfail=1 --ff {posargs}
 python -m coverage report -m --fail-under=100 --show-missing
- python ../scripts/validate_branch_check.py
+ python ./scripts/validate_branch_check.py


nit: why the leading ./ here and not on line 60?

alexwlchan · 2018-04-15T15:30:23Z

tooling/scripts/ensure-python.sh

+# This is to guard against multiple builds in parallel. The various installers will tend
+# to stomp all over each other if you do this and they haven't previously successfully
+# succeeded. We use a lock file to block progress so only one install runs at a time.
+# This script should be pretty fast once files are cached, so the lost of concurrency


nit: "lost" ~> "loss

alexwlchan · 2018-04-15T15:30:32Z

tooling/scripts/ensure-python.sh

+# is not a major problem.
+# This should be using the lockfile command, but that's not available on the
+# containerized travis and we can't install it without sudo.
+# Is is unclear if this is actually useful. I was seeing behaviour that suggested


nit: "It is unclear"

alexwlchan · 2018-04-15T15:31:15Z

tooling/scripts/ensure-python.sh

+# containerized travis and we can't install it without sudo.
+# Is is unclear if this is actually useful. I was seeing behaviour that suggested
+# concurrent runs of the installer, but I can't seem to find any evidence of this lock
+# ever not being acquired.


Surely a single Travis run only installs a single version of Python, in which case parallelism is never a problem. Is this only an issue for local builds?

I think so. When I wrote it it sure looked like it helped on Travis, but I think that was an illusion. I no longer remember the details.

alexwlchan · 2018-04-15T15:32:10Z

tooling/src/hypothesistooling/__init__.py

 """Given a task name, should we run this task?"""
- if not is_pull_request:
+ if not IS_PULL_REQUEST:


👍

much neater

alexwlchan

Some comments based on another partial read, but I haven't got to main.py yet.

DRMacIver · 2018-04-16T13:03:53Z

BTW the current status of this is that I'm struggling to reproduce the problem that is occurring reliably on CI where the install phase isn't actually working, and it's a rather slow loop to reproduce so I keep getting distracted by other work. Still ongoing work in progress though!

DRMacIver · 2018-04-16T14:31:50Z

BTW the current status of this is that I'm struggling to reproduce the problem that is occurring reliably on CI

(╯°□°）╯︵ ┻━┻

Eventually I figured out by staring at CI logs for long enough.

The problem is that when running a script that exists solely to read stuff out of the environment, maybe I shouldn't be carefully starting it in a blank environment. Ugh.

Zac-HD · 2018-04-16T15:09:56Z

tooling/src/hypothesistooling/__main__.py

- tools.ROOT, '.flake8'
- ))
+ pip_tool(
+ 'flake8',


Note: our current run requires flake8-docstrings to be installed too.

That's fine. This runs in a virtualenv that installs everything from tools.txt

Zac-HD · 2018-04-16T22:16:05Z

Looks like all you need to do is drop the old-python and old-pip tasks from Travis 😄

DRMacIver · 2018-04-18T14:33:37Z

Have fixed all of the small nits, but I think this is all good for review now.

alexwlchan · 2018-04-18T20:27:00Z

I have seen this, but I don’t have a lot of free time for OSS work right now, and I’m on holiday for a week from Sunday. I’d be surprised if I manage to review this properly within the next fortnight.

I won’t be upset if you want to merge this before I’ve reviewed it, and we patch it up after the fact.

Zac-HD · 2018-04-19T00:19:03Z

I'm not quite so busy, but just don't have the depth of experience with bash to do a useful review.

DRMacIver · 2018-04-19T07:33:56Z

I won’t be upset if you want to merge this before I’ve reviewed it, and we patch it up after the fact.

No worries. I'll definitely want to merge this in the next fortnight, but this is very much a stage one so it will probably see a lot of evolution from here, so you'll have plenty of later opportunity to comment. :-)

@froztbyte any chance you could take a look at this? It's rather in your area of expertise.

DRMacIver · 2018-04-19T07:34:57Z

just don't have the depth of experience with bash to do a useful review.

FWIW very little of the bash is actually new. It's all just extracted from existing bash scripts that have been more or less working for about four years now. That doesn't mean it's correct, but it's probably not broken in new ways, and it doesn't need a super extensive review.

alexwlchan

A few more small comments on the bash.

Given the inconsistent approaches to bash headers (#!/bin/bash and #!/usr/bin/env bash), and the varying uses of set -o or set -o errexit, maybe something that checks for common headers, similar to what we have for Python?

Just a thought.

alexwlchan · 2018-04-20T15:17:57Z

build.sh

+# version of Python, then hand over to the actual Hypothesis build runner (which
+# is written in Python instead of bash).
+
+set -x


nit: prefer “set -o xtrace” for readability

(and ease of Googling!)

alexwlchan · 2018-04-20T15:19:15Z

build.sh

+
+SCRIPTS="$ROOT/tooling/scripts"
+
+# shellcheck source=tooling/scripts/common.sh


minor: why is this commented out?

It's not a command, it's a shellcheck pragma to tell it where to look for the dynamic source.

alexwlchan · 2018-04-20T15:22:10Z

tooling/scripts/common.sh

+ROOT="$(git -C "$HERE" rev-parse --show-toplevel)"
+
+export ROOT
+export BUILD_RUNTIMES=${BUILD_RUNTIMES-$HOME/.cache/hypothesis-build-runtimes}


I’m familiar with assigning variables of the form:

export VARNAME=${VARIABLE:-default}

but this doesn't have the colon. What am I missing?

Both are valid syntax! Though it turns out with very slightly different semantics - :- will use the new value if VARIABLE is set but null, while - will use null in that case. I'll switch it over.

Both “Why bash? 😭” and “Why? bash 😭” are valid responses I had! Though it turns out with very slightly different semantics.

😉

alexwlchan · 2018-04-20T15:23:55Z

build.sh

+if [ ! -e "$TOOL_PYTHON" ] ; then
+ rm -rf "$TOOL_VIRTUALENV"
+ "$PYTHON" -m pip install --upgrade virtualenv
+ "$PYTHON" -m virtualenv "$TOOL_VIRTUALENV"


nit: is this extra indentation significant? It doesn't look to me like it is, but it makes the code slightly harder to follow.

Turns out to be mixed tabs and spaces from moving some stuff over from the makefile. Whoops!

DRMacIver · 2018-04-21T08:43:28Z

BTW I propose that at some point we just YOLO merge this rather than doing a detailed review.

Reasons:

It's large and annoying. I know, sorry.
A detailed review will conclude that it's not very good, but we know that already
Most of the details will change in smaller chunks as we monorepo more stuff, and that can be subject to more detailed review
It doesn't actually affect users because it doesn't touch any shippable code, so any problems with it will fall on us to fix later rather than inflicting them on other people.

In addition because I would like this merged sooner rather than later because:

We keep making changes to the Makefile. e.g. Rename private modules with leading underscore #1248.
I'm now working on hypothesis for Ruby again (and finding things that are arbitrarily wrong in its build environment!) and I'd like to bring it into the fold sooner rather than later so I can start getting some of the core under test in Python land.

Zac-HD

YOLO 🚀

I expect a follow-up issue to track the inevitable glitches, but let's get it in and iterate on a shared base.

DRMacIver · 2018-04-21T14:45:03Z

I expect a follow-up issue to track the inevitable glitches, but let's get it in and iterate on a shared base.

Oh for sure. But I mean hey things are still broken from the last big change, so how much worse can it be? 😉

Zac-HD · 2018-04-21T15:37:01Z

Things including certain build tasks are still broken, so... 😉

DRMacIver · 2018-04-21T15:42:17Z

Things including certain build tasks are still broken, so... 😉

Specifically there are several task that are broken in master but work correctly in this branch and so are picking up failures that are also present in master. 😉

Zac-HD · 2018-04-21T17:32:02Z

Merge when you think it's ready!

alexwlchan · 2018-04-21T17:36:55Z

BTW I propose that at some point we just YOLO merge this rather than doing a detailed review.

What Could Possibly Go Wrong.

DRMacIver · 2018-04-21T17:52:56Z

What Could Possibly Go Wrong.

Literally Everything.

DRMacIver force-pushed the DRMacIver/extract-tools branch from f412ad2 to 6e38365 Compare April 12, 2018 15:29

alexwlchan reviewed Apr 12, 2018

View reviewed changes

Zac-HD reviewed Apr 13, 2018

View reviewed changes

DRMacIver force-pushed the DRMacIver/extract-tools branch from f0053ff to d4cfecb Compare April 13, 2018 12:38

alexwlchan reviewed Apr 15, 2018

View reviewed changes

DRMacIver force-pushed the DRMacIver/extract-tools branch from 472a982 to e1bf16f Compare April 16, 2018 12:49

Zac-HD reviewed Apr 16, 2018

View reviewed changes

DRMacIver force-pushed the DRMacIver/extract-tools branch from f98202b to 1a7c223 Compare April 18, 2018 14:27

alexwlchan reviewed Apr 20, 2018

View reviewed changes

Zac-HD mentioned this pull request Apr 20, 2018

Add Python 3.7 to the build matrix #1150

Closed

DRMacIver added 3 commits April 21, 2018 09:31

Fix whitespace

9698e57

Change top level build system to custom runner

46ba1e5

Address shellcheck objections

550d97d

DRMacIver added 14 commits April 21, 2018 09:34

Don't warn about script paths

ddcad01

Less silly way of getting comon environment variables

f3f7eab

Get lint running on all Python files

298787f

Ditch tests for very old Python and Pip

4175779

Remove unused import

5c6ae24

Fix references to current script's dir

4724404

Fix nits

4d76f97

Fix typo

89c1592

Switch over a bunch of dirname -bash that I missed

c09f57c

Remove some unused imports

b7d5266

Remove weird indentation

063fe5c

More explicit form

1e6def7

Remove use of tabs

c34e517

Use other form for assign if not set

c09a835

DRMacIver force-pushed the DRMacIver/extract-tools branch from 94f49f8 to c09a835 Compare April 21, 2018 08:41

DRMacIver added 2 commits April 21, 2018 13:57

Elide long lists of files when printing commands

7cc6110

Clean up some flake8 errors

1a22ffe

Zac-HD approved these changes Apr 21, 2018

View reviewed changes

Add release file

678e4c5

Trailing whitespace

43495d7

DRMacIver merged commit b305ffd into master Apr 21, 2018

DRMacIver deleted the DRMacIver/extract-tools branch April 21, 2018 17:53

Zalathar mentioned this pull request Aug 20, 2018

CONTRIBUTING.rst has outdated references to the Makefile-based build system #1501

Closed


		SCRIPTS="$ROOT/tooling/scripts"

		# shellcheck source=tooling/scripts/common.sh

Move build system over to custom runner #1227

Move build system over to custom runner #1227

Conversation

DRMacIver commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexwlchan left a comment

Choose a reason for hiding this comment

DRMacIver commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexwlchan left a comment

Choose a reason for hiding this comment

DRMacIver commented Apr 16, 2018

DRMacIver commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD commented Apr 16, 2018

DRMacIver commented Apr 18, 2018

alexwlchan commented Apr 18, 2018

Zac-HD commented Apr 19, 2018

DRMacIver commented Apr 19, 2018

DRMacIver commented Apr 19, 2018

alexwlchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DRMacIver commented Apr 21, 2018 • edited Loading

Zac-HD left a comment

Choose a reason for hiding this comment

DRMacIver commented Apr 21, 2018

Zac-HD commented Apr 21, 2018

DRMacIver commented Apr 21, 2018

Zac-HD commented Apr 21, 2018

alexwlchan commented Apr 21, 2018

DRMacIver commented Apr 21, 2018

DRMacIver commented Apr 21, 2018 •

edited

Loading