Travis CI: Add tests to find syntax errors and undefined names

[cclauss]

Discovered and documented #35 ... __allow_failures__ can be turned off once #35 is fixed.

[snarkmaster]

Thank you for your contribution, @cclaus! Closing out for reasons below — if you'd like to put up a working fix for exec, please either reopen the PR with a 1-file change, or make a new one. I would be OK with converting that entire file to Py3 wholesale, I just have not made it a priority. I have provided a test case below.

Adding flake8 to the Travis build

I'm not deeply against this, but the tradeoff is that this makes the build slightly slower, and our Travis build is already prone to timing out due to the insane amount of C++ it has to compile.

Internally at FB, we run significantly stricter flake8 rules, which go off the file hashbang line to determine the intended Python flavor. Issue #35 is pure human error, I suspect the author didn't intend the file to ever run under Python2, and failed to mark the file as Python3-only. I have reached out to them, and will get this fixed. This will be tracked on #35.

Converting exec from a statement to a function

Unfortunately, these are not equivalent in Python2, so the new version does not work:

.../bistro$ ./cmake/targets_to_cmake_lists.py  .
  File "./cmake/targets_to_cmake_lists.py", line 156
    exec(s, {l: fn_locals[l] for l in (
SyntaxError: unqualified exec is not allowed in function 'parse_targets' it contains a nested function with free variables

Conditionally import typing

As with the #35, this is not a part of Bistro, and not used by Bistro. I have reached out to the maintainer of that code, but he is out on vacation, so this may take some time in getting fixed.

[cclauss]

Thank you for your comments, @snarkmaste!

please either reopen the PR

When a pull request is closed by someone with the commit bit then it can not be reopened by someone without the commit bit. I created #36 to deal with the exec syntax error in isolation.

I'm not deeply against [adding the flake8 tests], but the tradeoff is that this makes the build slightly slower, and our Travis build is already prone to timing out due to the insane amount of C++ it has to compile.

Looking at the Travis runs note that each of the two flake8 runs lint the entire codebase in less than 40 seconds compared to the 15+ minutes required to execute the C++ job. More importantly, note that these jobs are run in parallel so they do not slow down the build. The flake8 jobs should always start after the C++ job and finish before the C++ job ends.

Does the use of __travis_wait__ help to reduce the problem of the C++ job timing out?

[snarkmaster]

reopen the PR

Sorry, I wasn't familiar with that caveat. Thanks for making a new one.

flake8 runs in parallel

Well, it still eats up CPU & IO resources in the Travis container, so while your assertion that it would start later and finish earlier is true, I strongly suspect it would slow down the job.

I'd be much more excited about adding it if Facebook's internal linters didn't already run flake8 with much stricter settings.

travis_wait

That would help if the build were timing out due to lack of output.

Unfortunately, our build times out because we are building megabytes upon megabytes of C++, and so a full build just takes longer than Travis allows.

I have a fairly complex workaround for this that reuses build artifacts across runs using ccache — without it, the build would never finish within 40 minutes.

[cclauss]

Understood but If the internal linters running flake8 with much stricter settings had actually caught the syntax error in __bistro/cmake/targets_to_cmake_lists.py__ then these PRs would not exist.

[snarkmaster]

Oh, @cclaus, I think I misunderstood your earlier comment. Those flake8 runs actually execute in separate Travis jobs, right? Then my concerns about CPU/memory contention would likely be effectively moot, since Travis does allow multiple jobs to consume multiple hours of wall time.