Conversation
|
The try build times out for some reason, https://travis-ci.org/WebAssembly/binaryen/jobs/167373236, but not quite sure why that would happen? Has there been flakiness like this with travis before? |
check.py
Outdated
| # limitations under the License. | ||
|
|
||
| import os, shutil, sys, subprocess, difflib, json, time, urllib2 | ||
| import os, shutil, sys, subprocess, difflib, json, time, urllib2, optparse |
There was a problem hiding this comment.
We should probably use argparse instead of the obsolete optparse. I think it should be a pretty minor change from what we have here since it looks like we're just using the basic features.
There was a problem hiding this comment.
optparse is not obsolete, it is just no longer developed.. but sure, I'll convert.
check.py
Outdated
|
|
||
| usage_str = "usage: 'python check.py [options]'\n\n Runs the Binaryen test suite." | ||
| parser = optparse.OptionParser(usage=usage_str) | ||
| parser.add_option('--torture', dest='torture', action='store_true', default=True, help='Chooses whether to run the torture testcases. Default: true.') |
There was a problem hiding this comment.
It seems unnecessary to have these options which are just on by default. It seems like the only use case would be just to allow them to appear after the negation versions to turn the option back on again?
There was a problem hiding this comment.
Agree, but there was an existing --torture parameter, so I did not want to remove that or some existing users could break. Wanted to be systematic then.
There was a problem hiding this comment.
I'm pretty sure we don't use that for our automated tests or local development, and I don't think it's used in Binaryen's CI (which you are obviously updating), so if @kripken doesn't use it commonly, I don't think breakage is a big concern for that particular flag.
| else: | ||
| options.binaryen_bin = 'bin' | ||
|
|
||
| if not os.path.isfile(os.path.join(options.binaryen_bin, 'wasm-dis')) and not os.path.isfile(os.path.join(options.binaryen_bin, 'wasm-dis.exe')): |
There was a problem hiding this comment.
Seems like we could factor this kind of thing into some function like ExecutableName() which might append .exe on Windows and nothing otherwise?
There was a problem hiding this comment.
Considered that, but then settled for this since it's only a single location in the script.
There was a problem hiding this comment.
OK, we can factor it if we end up needing it in more places.
check.py
Outdated
| has_node = False | ||
| try: | ||
| subprocess.check_call(['nodejs', '--version'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
| subprocess.check_call(['nodejs', '--version'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True) |
There was a problem hiding this comment.
We are doing similar Windows work on the waterfall script currently as well, as we're bringing up mac and Windows builds, and I was just looking at a similar change there. Our script infrastructure has a few batch files which we found we needed shell=True for that; but I don't think any of these are batch files, and I'd rather not have shell=True if we don't have to. I assume it's in this PR for a reason though, what makes it necessary here?
There was a problem hiding this comment.
The issue is with the suffixes, which may be emcc, emcc.bat, or nodejs.exe, which this doesn't specify. The suffixes are autodetected only if shell=True, otherwise it does not look for nodejs.exe when asking to run nodejs.
There was a problem hiding this comment.
OK; this is fine for now. I'll think a little more as I update the waterfall scripts and if we come up with a better way than just having shell=True everywhere then we can update everything.
There was a problem hiding this comment.
OK, so the travis hang reproduces for me locally, and it's caused by adding shell=True here. Not immediately obvious why.
There was a problem hiding this comment.
I did some experiments on Linux. Apparently shell=True has some effect on the stdin used by the process; if you feed something in, e.g. stdin=open('/dev/null') it works. Maybe nodejs is trying to read something from stdin?. shell=True also affects stdout; i.e. if you set your command to something like ['echo','foo'] the output will appear on your console with shell=False but not with shell=True. In other words, stdin/out/err are supposed to be inherited by the child process when not using redirection, but apparently shell=True has some other behavior (that doesn't seem to be documented as far as I can tell).
Anyway it's odd and magical and makes me want to avoid shell=True even more :-/. Unfortunately although Python has lots of good shell-like tools, I don't think an equivalent to which/where is among them. However it's fairly straightforward to write: see e.g. https://chromium.googlesource.com/native_client/src/native_client/+/master/pynacl/file_tools.py#72
There was a problem hiding this comment.
I've usually seen this written as shell=is_windows. But yeah, the whole thing is pretty horrible. :-|
There was a problem hiding this comment.
Alright, thanks for checking that. I'll refactor to avoid the shell=Trues.
|
I have seen random timeouts on Travis before :( |
juj
force-pushed
the
more_flexible_check_py
branch
from
2822e48 to
bf1e8a9
Compare
|
Pushed the change to |
|
Hm, maybe the travis thing is real :( let me try it locally. |
juj
force-pushed
the
more_flexible_check_py
branch
from
bf1e8a9 to
729293a
Compare
|
Yay, passes travis now. How does this look? |
|
lgtm |
check.py
Outdated
| WATERFALL_BUILD_DIR = os.path.join(options.binaryen_test, 'wasm-install') | ||
| BIN_DIR = os.path.abspath(os.path.join(WATERFALL_BUILD_DIR, 'wasm-install', 'bin')) | ||
|
|
||
| GCC = os.environ.get('CC') or which('mingw32-gcc') or which('gcc') or '' |
There was a problem hiding this comment.
How about we add clang here, so maybe this will work on mac too? And I guess if we're going to do that we could call it HOSTCC or NATIVECC or something like that instead, since it doesn't matter that it's gcc, just that it's a compiler that targets the host/build machine rather than wasm.
…e executed in different environments. Relates to WebAssembly#762.
juj
force-pushed
the
more_flexible_check_py
branch
from
729293a to
fb0648c
Compare
Normally this option default to false, and it seems like binaryen doesn't have a good reason to differ. However, this setting (originally called `--abort-on-first-failure`) has always been enabled by default since its was first added back in #771. Also name the option `--failfast` to match the name it has in the python unittest framework.
4 participants
Fix check.py to run on Windows and improve it to be configurable to be executed in different environments. Relates to #762.