<div dir="ltr">LGTM.<div><br></div><div> - Daniel</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 9, 2013 at 2:34 PM, Chris Matthews <span dir="ltr"><<a href="mailto:chris.matthews@apple.com" target="_blank">chris.matthews@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Thanks for the fast review!<div><br></div><div>Here are your proposed changes:</div><div>
<br></div><div><br></div><div><div>diff --git a/lnt/testing/util/commands.py b/lnt/testing/util/commands.py</div><div>index 39d12ea198791a58aeed230193bc5404580465de..4f0d2e8f489659905f9098e201415c4b83b97de6 100644</div><div>
--- a/lnt/testing/util/commands.py</div><div>+++ b/lnt/testing/util/commands.py</div><div>@@ -94,3 +94,29 @@ def which(command, paths = None):</div><div> return p</div><div> </div><div> return None</div>
<div class="im"><div>+</div><div>+def resolve_command_path(name):</div><div>+ """Try to make the name/path given into an absolute path to an</div><div>+ executable.</div><div>+</div><div>+ """</div>
<div>+ # If the given name exists (or is a path), make it absolute.</div><div>+ if os.path.exists(name):</div><div>+ return os.path.abspath(name)</div><div>+</div><div>+ # Otherwise we most likely have a command name, try to look it up.</div>
<div>+ path = which(name)</div><div>+ if path is not None:</div><div>+ note("resolved command %r to path %r" % (name, path))</div><div>+ return path</div><div>+</div><div>+ # If that failed just return the original name.</div>
<div>+ return name</div></div><div>+</div><div>+def isexecfile(path):</div><div class="im"><div>+ """Does this path point to a valid executable?</div><div>+</div></div><div>+ """</div>
<div class="im"><div>+ return os.path.isfile(path) and os.access(path, os.X_OK)</div><div>+</div></div><div class="im"><div>+</div><div>diff --git a/lnt/tests/compile.py b/lnt/tests/compile.py</div></div><div>index 77a235da7fcfb55b558ef246ef71acf5d1531d94..69a7cc5db122b260d5e9f634a8caaa3dffaaed54 100644</div>
<div>--- a/lnt/tests/compile.py</div><div>+++ b/lnt/tests/compile.py</div><div>@@ -15,7 +15,9 @@ from datetime import datetime</div><div> </div><div> import lnt.testing</div><div> import lnt.testing.util.compilers</div><div>
-from lnt.testing.util.commands import note, warning, error, fatal</div><div>+from lnt.testing.util.commands import note, warning, error, fatal, \</div><div>+ resolve_command_path, isexecfile</div><div class="im"><div>
+</div><div> from lnt.testing.util.misc import TeeStream, timestamp</div><div> from lnt.testing.util import commands, machineinfo</div><div> from lnt.util import stats</div></div><div>@@ -750,6 +752,13 @@ class CompileTest(builtintest.BuiltinTest):</div>
<div class="im"><div> if len(args) != 0:</div><div> parser.error("invalid number of arguments")</div><div> </div><div>+ # Resolve the cc_under_test path.</div><div>+ <a href="http://opts.cc" target="_blank">opts.cc</a> = resolve_command_path(<a href="http://opts.cc" target="_blank">opts.cc</a>)</div>
<div>+</div></div><div>+ if not isexecfile(<a href="http://opts.cc" target="_blank">opts.cc</a>):</div><div class="im"><div>+ parser.error('--cc does not point to a valid executable.')</div><div>
+</div><div>+</div><div> # Attempt to infer the cxx compiler if not given.</div><div> if <a href="http://opts.cc" target="_blank">opts.cc</a> and opts.cxx is None:</div><div> opts.cxx = lnt.testing.util.compilers.infer_cxx_compiler(<a href="http://opts.cc" target="_blank">opts.cc</a>)</div>
<div>diff --git a/lnt/tests/nt.py b/lnt/tests/nt.py</div></div><div>index cefb38c42c7c05cc24df76bc263e93cb50b7b09b..cb138cb51d235af73f5df4d516532cd05de7e5b0 100644</div><div>--- a/lnt/tests/nt.py</div><div>+++ b/lnt/tests/nt.py</div>
<div>@@ -14,8 +14,8 @@ from datetime import datetime</div><div> import lnt.testing</div><div> import lnt.testing.util.compilers</div><div> </div><div>-from lnt.testing.util.commands import note, warning, error, fatal</div>
<div class="im"><div>-from lnt.testing.util.commands import capture, mkdir_p, which</div></div><div>+from lnt.testing.util.commands import note, warning, error, fatal, \</div><div>+ capture, mkdir_p, resolve_command_path, isexecfile</div>
<div class="im"><div> from lnt.testing.util.rcs import get_source_version</div><div> from lnt.testing.util.misc import timestamp</div><div> </div></div><div>@@ -387,20 +387,6 @@ class TestConfiguration(object):</div><div class="im">
<div> </div><div> ###</div><div> </div><div>-def resolve_command_path(name):</div><div>- # If the given name exists (or is a path), make it absolute.</div><div>- if os.path.exists(name):</div><div>- return os.path.abspath(name)</div>
<div>-</div><div>- # Otherwise we most likely have a command name, try to look it up.</div><div>- path = which(name)</div><div>- if path is not None:</div><div>- note("resolved command %r to path %r" % (name, path))</div>
<div>- return path</div><div>-</div><div>- # If that failed just return the original name.</div><div>- return name</div><div>-</div><div> def scan_for_test_modules(config):</div><div> base_modules_path = os.path.join(config.test_suite_root, 'LNTBased')</div>
<div> if config.only_test is None:</div></div><div>@@ -1339,6 +1325,9 @@ class NTTest(builtintest.BuiltinTest):</div><div class="im"><div> # Resolve the cc_under_test path.</div><div> <a href="http://opts.cc" target="_blank">opts.cc</a>_under_test = resolve_command_path(<a href="http://opts.cc" target="_blank">opts.cc</a>_under_test)</div>
<div> </div></div><div>+ if not isexecfile(<a href="http://opts.cc" target="_blank">opts.cc</a>_under_test):</div><div class="im"><div>+ parser.error('--cc does not point to a valid executable.')</div>
<div>+</div><div> # If there was no --cxx given, attempt to infer it from the --cc.</div><div> if opts.cxx_under_test is None:</div><div> opts.cxx_under_test = lnt.testing.util.compilers.infer_cxx_compiler(</div>
<div><br></div><div><br></div><div> </div><div>
<div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px"><div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px">
<div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px"><div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px">
</div></div></div></div></div></div></div></div><br><div style="word-wrap:break-word"><div><div><div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px">
<div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px"><div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px">
<div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px"><br><span style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:start;font-style:normal;display:inline!important;font-weight:normal;float:none;line-height:normal;text-transform:none;white-space:normal;font-family:Helvetica;word-spacing:0px">Chris Matthews</span><br style="line-height:normal;text-indent:0px;letter-spacing:normal;text-align:start;font-variant:normal;text-transform:none;font-style:normal;white-space:normal;font-family:Helvetica;font-weight:normal;word-spacing:0px">
<span style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:start;font-style:normal;display:inline!important;font-weight:normal;float:none;line-height:normal;text-transform:none;white-space:normal;font-family:Helvetica;word-spacing:0px"><a href="mailto:chris.matthews@apple.com" target="_blank">chris.matthews@apple.com</a></span><br style="line-height:normal;text-indent:0px;letter-spacing:normal;text-align:start;font-variant:normal;text-transform:none;font-style:normal;white-space:normal;font-family:Helvetica;font-weight:normal;word-spacing:0px">
</div></div></div></div>
</div>
<br><div><div>On Sep 9, 2013, at 10:40 AM, Daniel Dunbar <<a href="mailto:daniel@zuster.org" target="_blank">daniel@zuster.org</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr">On Mon, Sep 9, 2013 at 10:24 AM, Chris Matthews <span dir="ltr"><<a href="mailto:chris.matthews@apple.com" target="_blank">chris.matthews@apple.com</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word">
<div style="text-align:start;text-indent:0px;word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word">When you specified something odd for --cc you'd get a really cryptic "Permission Denied" error when we go to check the compiler version. Do better checking on the --cc executable first, and error if it is not 1) executable and 2) a file. Also, make the compile test function use the same compiler resolution procedure, including this error.</div>
<div style="text-align:start;text-indent:0px;word-wrap:break-word"><br></div><div style="text-align:start;text-indent:0px;word-wrap:break-word">diff --git a/lnt/testing/util/compilers.py b/lnt/testing/util/compilers.py<br>
index 6b8f0348c7b21865c576374c38d2d4b13c1b51ec..ffdd061edbf5add2fd7c119c78bb1fb76b6b7df2 100644<br>--- a/lnt/testing/util/compilers.py<br>+++ b/lnt/testing/util/compilers.py<br>@@ -13,6 +13,12 @@ def ishexhash(string):<br>
for c in string<br> if c.isdigit() or c in 'abcdef']) == 40<br> <br>+<br>+def is_valid(path):<br>+ """Does this path point to a valid executable?"""<br>
+ return os.path.isfile(path) and os.access(path, os.X_OK)<br>
+<br></div></div></div></div></div></blockquote><div><br></div><div>I would just call this isexecfile() and put it into lnt.util.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word">
<div style="text-align:start;text-indent:0px;word-wrap:break-word">+<br> def get_cc_info(path, cc_flags=[]):<br> """get_cc_info(path) -> { ... }<br> <br>diff --git a/lnt/tests/common.py b/lnt/tests/common.py<br>
new file mode 100644<br>index 0000000000000000000000000000000000000000..23a6b11bfb40b645e1ccd2a834c796bdd409483c<br>--- /dev/null<br>+++ b/lnt/tests/common.py<br>@@ -0,0 +1,24 @@<br>+"""Some functions which are common to test suites."""<br>
+import os<br>+from lnt.testing.util.commands import capture, mkdir_p<br>+<br>+<br>+def resolve_command_path(name):<br>+ """Try to make the name/path given into an absolute path to an<br>+ executable.<br>
+<br>+ """<br>+ # If the given name exists (or is a path), make it absolute.<br>+ if os.path.exists(name):<br>+ return os.path.abspath(name)<br>+<br>+ # Otherwise we most likely have a command name, try to look it up.<br>
+ path = which(name)<br>+ if path is not None:<br>+ note("resolved command %r to path %r" % (name, path))<br>+ return path<br>+<br>+ # If that failed just return the original name.<br>+ return name<br>
</div></div></div></div></div></blockquote><div><br></div><div>This function is generic enough I think it should also just go into lnt.util and avoid creating another "util-like" module.</div><div><br></div>
<div> - Daniel</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word">
<div style="text-align:start;text-indent:0px;word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word">+<br>+<br>diff --git a/lnt/tests/compile.py b/lnt/tests/compile.py<br>
index 77a235da7fcfb55b558ef246ef71acf5d1531d94..eaa4f9bea34fc986d3875a70fe6eaac34f27ba32 100644<br>--- a/lnt/tests/compile.py<br>+++ b/lnt/tests/compile.py<br>@@ -19,6 +19,7 @@ from lnt.testing.util.commands import note, warning, error, fatal<br>
from lnt.testing.util.misc import TeeStream, timestamp<br> from lnt.testing.util import commands, machineinfo<br> from lnt.util import stats<br>+from lnt.tests.common import resolve_command_path<br> <br> def args_to_quoted_string(args):<br>
def quote_arg(arg):<br>@@ -750,6 +751,13 @@ class CompileTest(builtintest.BuiltinTest):<br> if len(args) != 0:<br> parser.error("invalid number of arguments")<br> <br>+ # Resolve the cc_under_test path.<br>
+ <a href="http://opts.cc/" target="_blank">opts.cc</a> = resolve_command_path(<a href="http://opts.cc/" target="_blank">opts.cc</a>)<br>+<br>+ if not lnt.testing.util.compilers.is_valid(<a href="http://opts.cc/" target="_blank">opts.cc</a>):<br>
+ parser.error('--cc does not point to a valid executable.')<br>+<br>+<br> # Attempt to infer the cxx compiler if not given.<br> if <a href="http://opts.cc/" target="_blank">opts.cc</a> and opts.cxx is None:<br>
opts.cxx = lnt.testing.util.compilers.infer_cxx_compiler(<a href="http://opts.cc/" target="_blank">opts.cc</a>)<br>diff --git a/lnt/tests/nt.py b/lnt/tests/nt.py<br>index cefb38c42c7c05cc24df76bc263e93cb50b7b09b..fd274e19bb22b27d2e535b2e7653e6ac0069823b 100644<br>
--- a/lnt/tests/nt.py<br>+++ b/lnt/tests/nt.py<br>@@ -14,8 +14,9 @@ from datetime import datetime<br> import lnt.testing<br> import lnt.testing.util.compilers<br> <br>+from lnt.tests.common import resolve_command_path<br>
from lnt.testing.util.commands import note, warning, error, fatal<br>-from lnt.testing.util.commands import capture, mkdir_p, which<br>+from lnt.testing.util.commands import capture, mkdir_p<br> from lnt.testing.util.rcs import get_source_version<br>
from lnt.testing.util.misc import timestamp<br> <br>@@ -387,20 +388,6 @@ class TestConfiguration(object):<br> <br> ###<br> <br>-def resolve_command_path(name):<br>- # If the given name exists (or is a path), make it absolute.<br>
- if os.path.exists(name):<br>- return os.path.abspath(name)<br>-<br>- # Otherwise we most likely have a command name, try to look it up.<br>- path = which(name)<br>- if path is not None:<br>- note("resolved command %r to path %r" % (name, path))<br>
- return path<br>-<br>- # If that failed just return the original name.<br>- return name<br>-<br> def scan_for_test_modules(config):<br> base_modules_path = os.path.join(config.test_suite_root, 'LNTBased')<br>
if config.only_test is None:<br>@@ -1339,6 +1326,9 @@ class NTTest(builtintest.BuiltinTest):<br> # Resolve the cc_under_test path.<br> <a href="http://opts.cc/" target="_blank">opts.cc</a>_under_test = resolve_command_path(<a href="http://opts.cc/" target="_blank">opts.cc</a>_under_test)<br>
<br>+ if not lnt.testing.util.compilers.is_valid(<a href="http://opts.cc/" target="_blank">opts.cc</a>_under_test):<br>+ parser.error('--cc does not point to a valid executable.')<br>+<br> # If there was no --cxx given, attempt to infer it from the --cc.<br>
if opts.cxx_under_test is None:<br> opts.cxx_under_test = lnt.testing.util.compilers.infer_cxx_compiler(<br><br></div><div style="text-align:start;text-indent:0px;word-wrap:break-word"></div></div></div>
</div></div><br><div style="word-wrap:break-word"><div><div style="text-align:start;text-indent:0px;word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word"><div style="text-align:start;text-indent:0px;word-wrap:break-word">
<div style="text-align:start;text-indent:0px;word-wrap:break-word"></div><div style="text-align:start;text-indent:0px;word-wrap:break-word"><br><span style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:start;font-style:normal;display:inline!important;font-weight:normal;float:none;line-height:normal;text-transform:none;white-space:normal;font-family:Helvetica;word-spacing:0px">Chris Matthews</span><br style="line-height:normal;text-indent:0px;letter-spacing:normal;text-align:start;font-variant:normal;text-transform:none;font-style:normal;white-space:normal;font-family:Helvetica;font-weight:normal;word-spacing:0px">
<span style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:start;font-style:normal;display:inline!important;font-weight:normal;float:none;line-height:normal;text-transform:none;white-space:normal;font-family:Helvetica;word-spacing:0px"><a href="mailto:chris.matthews@apple.com" target="_blank">chris.matthews@apple.com</a></span><br style="line-height:normal;text-indent:0px;letter-spacing:normal;text-align:start;font-variant:normal;text-transform:none;font-style:normal;white-space:normal;font-family:Helvetica;font-weight:normal;word-spacing:0px">
</div></div></div></div>
</div>
<br></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>
</blockquote></div><br></div></div><br></blockquote></div><br></div>