[LNT][PATCH] Better errors for invalid --cc parameters.
Daniel Dunbar
daniel at zuster.org
Mon Sep 9 15:07:48 PDT 2013
LGTM.
- Daniel
On Mon, Sep 9, 2013 at 2:34 PM, Chris Matthews <chris.matthews at apple.com>wrote:
> 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/acda1994/attachment.html>
More information about the llvm-commits
mailing list