[LNT][PATCH] Better errors for invalid --cc parameters.
Chris Matthews
chris.matthews at apple.com
Mon Sep 9 14:34:52 PDT 2013
Thanks for the fast review!
Here are your proposed changes:
diff --git a/lnt/testing/util/commands.py b/lnt/testing/util/commands.py
index 39d12ea198791a58aeed230193bc5404580465de..4f0d2e8f489659905f9098e201415c4b83b97de6 100644
--- a/lnt/testing/util/commands.py
+++ b/lnt/testing/util/commands.py
@@ -94,3 +94,29 @@ def which(command, paths = None):
return p
return None
+
+def resolve_command_path(name):
+ """Try to make the name/path given into an absolute path to an
+ executable.
+
+ """
+ # If the given name exists (or is a path), make it absolute.
+ if os.path.exists(name):
+ return os.path.abspath(name)
+
+ # Otherwise we most likely have a command name, try to look it up.
+ path = which(name)
+ if path is not None:
+ note("resolved command %r to path %r" % (name, path))
+ return path
+
+ # If that failed just return the original name.
+ return name
+
+def isexecfile(path):
+ """Does this path point to a valid executable?
+
+ """
+ return os.path.isfile(path) and os.access(path, os.X_OK)
+
+
diff --git a/lnt/tests/compile.py b/lnt/tests/compile.py
index 77a235da7fcfb55b558ef246ef71acf5d1531d94..69a7cc5db122b260d5e9f634a8caaa3dffaaed54 100644
--- a/lnt/tests/compile.py
+++ b/lnt/tests/compile.py
@@ -15,7 +15,9 @@ from datetime import datetime
import lnt.testing
import lnt.testing.util.compilers
-from lnt.testing.util.commands import note, warning, error, fatal
+from lnt.testing.util.commands import note, warning, error, fatal, \
+ resolve_command_path, isexecfile
+
from lnt.testing.util.misc import TeeStream, timestamp
from lnt.testing.util import commands, machineinfo
from lnt.util import stats
@@ -750,6 +752,13 @@ class CompileTest(builtintest.BuiltinTest):
if len(args) != 0:
parser.error("invalid number of arguments")
+ # Resolve the cc_under_test path.
+ opts.cc = resolve_command_path(opts.cc)
+
+ if not isexecfile(opts.cc):
+ parser.error('--cc does not point to a valid executable.')
+
+
# Attempt to infer the cxx compiler if not given.
if opts.cc and opts.cxx is None:
opts.cxx = lnt.testing.util.compilers.infer_cxx_compiler(opts.cc)
diff --git a/lnt/tests/nt.py b/lnt/tests/nt.py
index cefb38c42c7c05cc24df76bc263e93cb50b7b09b..cb138cb51d235af73f5df4d516532cd05de7e5b0 100644
--- a/lnt/tests/nt.py
+++ b/lnt/tests/nt.py
@@ -14,8 +14,8 @@ from datetime import datetime
import lnt.testing
import lnt.testing.util.compilers
-from lnt.testing.util.commands import note, warning, error, fatal
-from lnt.testing.util.commands import capture, mkdir_p, which
+from lnt.testing.util.commands import note, warning, error, fatal, \
+ capture, mkdir_p, resolve_command_path, isexecfile
from lnt.testing.util.rcs import get_source_version
from lnt.testing.util.misc import timestamp
@@ -387,20 +387,6 @@ class TestConfiguration(object):
###
-def resolve_command_path(name):
- # If the given name exists (or is a path), make it absolute.
- if os.path.exists(name):
- return os.path.abspath(name)
-
- # Otherwise we most likely have a command name, try to look it up.
- path = which(name)
- if path is not None:
- note("resolved command %r to path %r" % (name, path))
- return path
-
- # If that failed just return the original name.
- return name
-
def scan_for_test_modules(config):
base_modules_path = os.path.join(config.test_suite_root, 'LNTBased')
if config.only_test is None:
@@ -1339,6 +1325,9 @@ class NTTest(builtintest.BuiltinTest):
# Resolve the cc_under_test path.
opts.cc_under_test = resolve_command_path(opts.cc_under_test)
+ if not isexecfile(opts.cc_under_test):
+ parser.error('--cc does not point to a valid executable.')
+
# If there was no --cxx given, attempt to infer it from the --cc.
if opts.cxx_under_test is None:
opts.cxx_under_test = lnt.testing.util.compilers.infer_cxx_compiler(
Chris Matthews
chris.matthews at apple.com
On Sep 9, 2013, at 10:40 AM, Daniel Dunbar <daniel at zuster.org> wrote:
> On Mon, Sep 9, 2013 at 10:24 AM, Chris Matthews <chris.matthews at apple.com> wrote:
> 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.
>
> diff --git a/lnt/testing/util/compilers.py b/lnt/testing/util/compilers.py
> index 6b8f0348c7b21865c576374c38d2d4b13c1b51ec..ffdd061edbf5add2fd7c119c78bb1fb76b6b7df2 100644
> --- a/lnt/testing/util/compilers.py
> +++ b/lnt/testing/util/compilers.py
> @@ -13,6 +13,12 @@ def ishexhash(string):
> for c in string
> if c.isdigit() or c in 'abcdef']) == 40
>
> +
> +def is_valid(path):
> + """Does this path point to a valid executable?"""
> + return os.path.isfile(path) and os.access(path, os.X_OK)
> +
>
> I would just call this isexecfile() and put it into lnt.util.
>
> +
> def get_cc_info(path, cc_flags=[]):
> """get_cc_info(path) -> { ... }
>
> diff --git a/lnt/tests/common.py b/lnt/tests/common.py
> new file mode 100644
> index 0000000000000000000000000000000000000000..23a6b11bfb40b645e1ccd2a834c796bdd409483c
> --- /dev/null
> +++ b/lnt/tests/common.py
> @@ -0,0 +1,24 @@
> +"""Some functions which are common to test suites."""
> +import os
> +from lnt.testing.util.commands import capture, mkdir_p
> +
> +
> +def resolve_command_path(name):
> + """Try to make the name/path given into an absolute path to an
> + executable.
> +
> + """
> + # If the given name exists (or is a path), make it absolute.
> + if os.path.exists(name):
> + return os.path.abspath(name)
> +
> + # Otherwise we most likely have a command name, try to look it up.
> + path = which(name)
> + if path is not None:
> + note("resolved command %r to path %r" % (name, path))
> + return path
> +
> + # If that failed just return the original name.
> + return name
>
> This function is generic enough I think it should also just go into lnt.util and avoid creating another "util-like" module.
>
> - Daniel
>
> +
> +
> diff --git a/lnt/tests/compile.py b/lnt/tests/compile.py
> index 77a235da7fcfb55b558ef246ef71acf5d1531d94..eaa4f9bea34fc986d3875a70fe6eaac34f27ba32 100644
> --- a/lnt/tests/compile.py
> +++ b/lnt/tests/compile.py
> @@ -19,6 +19,7 @@ from lnt.testing.util.commands import note, warning, error, fatal
> from lnt.testing.util.misc import TeeStream, timestamp
> from lnt.testing.util import commands, machineinfo
> from lnt.util import stats
> +from lnt.tests.common import resolve_command_path
>
> def args_to_quoted_string(args):
> def quote_arg(arg):
> @@ -750,6 +751,13 @@ class CompileTest(builtintest.BuiltinTest):
> if len(args) != 0:
> parser.error("invalid number of arguments")
>
> + # Resolve the cc_under_test path.
> + opts.cc = resolve_command_path(opts.cc)
> +
> + if not lnt.testing.util.compilers.is_valid(opts.cc):
> + parser.error('--cc does not point to a valid executable.')
> +
> +
> # Attempt to infer the cxx compiler if not given.
> if opts.cc and opts.cxx is None:
> opts.cxx = lnt.testing.util.compilers.infer_cxx_compiler(opts.cc)
> diff --git a/lnt/tests/nt.py b/lnt/tests/nt.py
> index cefb38c42c7c05cc24df76bc263e93cb50b7b09b..fd274e19bb22b27d2e535b2e7653e6ac0069823b 100644
> --- a/lnt/tests/nt.py
> +++ b/lnt/tests/nt.py
> @@ -14,8 +14,9 @@ from datetime import datetime
> import lnt.testing
> import lnt.testing.util.compilers
>
> +from lnt.tests.common import resolve_command_path
> from lnt.testing.util.commands import note, warning, error, fatal
> -from lnt.testing.util.commands import capture, mkdir_p, which
> +from lnt.testing.util.commands import capture, mkdir_p
> from lnt.testing.util.rcs import get_source_version
> from lnt.testing.util.misc import timestamp
>
> @@ -387,20 +388,6 @@ class TestConfiguration(object):
>
> ###
>
> -def resolve_command_path(name):
> - # If the given name exists (or is a path), make it absolute.
> - if os.path.exists(name):
> - return os.path.abspath(name)
> -
> - # Otherwise we most likely have a command name, try to look it up.
> - path = which(name)
> - if path is not None:
> - note("resolved command %r to path %r" % (name, path))
> - return path
> -
> - # If that failed just return the original name.
> - return name
> -
> def scan_for_test_modules(config):
> base_modules_path = os.path.join(config.test_suite_root, 'LNTBased')
> if config.only_test is None:
> @@ -1339,6 +1326,9 @@ class NTTest(builtintest.BuiltinTest):
> # Resolve the cc_under_test path.
> opts.cc_under_test = resolve_command_path(opts.cc_under_test)
>
> + if not lnt.testing.util.compilers.is_valid(opts.cc_under_test):
> + parser.error('--cc does not point to a valid executable.')
> +
> # If there was no --cxx given, attempt to infer it from the --cc.
> if opts.cxx_under_test is None:
> opts.cxx_under_test = lnt.testing.util.compilers.infer_cxx_compiler(
>
>
>
> Chris Matthews
> chris.matthews at apple.com
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130909/ba36657d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_commit_b7a042a1d6b1.patch
Type: application/octet-stream
Size: 4009 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130909/ba36657d/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130909/ba36657d/attachment-0001.html>
More information about the llvm-commits
mailing list