[Mesa-dev] [PATCH] SConstruct: Fix PEP 8 issues.
Jose Fonseca
jfonseca at vmware.com
Wed Mar 18 03:38:40 PDT 2015
Hi Vinson,
I have mixed feelings about this.
I don't really agree with all things mandated with PEP 8. Comments inline.
On 17/03/15 06:15, Vinson Lee wrote:
> Signed-off-by: Vinson Lee <vlee at freedesktop.org>
> ---
> SConstruct | 66 ++++++++++++++++++++++++++++++++------------------------------
> 1 file changed, 34 insertions(+), 32 deletions(-)
>
> diff --git a/SConstruct b/SConstruct
> index ef71ab6..f9d3d4c 100644
> --- a/SConstruct
> +++ b/SConstruct
> @@ -1,7 +1,7 @@
> #######################################################################
> # Top-level SConstruct
> #
> -# For example, invoke scons as
> +# For example, invoke scons as
> #
> # scons build=debug llvm=yes machine=x86
> #
> @@ -12,13 +12,13 @@
> # build='debug'
> # llvm=True
> # machine='x86'
> -#
> +#
> # Invoke
> #
> # scons -h
> #
> # to get the full list of options. See scons manpage for more info.
> -#
> +#
>
> import os
> import os.path
> @@ -34,14 +34,14 @@ opts = Variables('config.py')
> common.AddOptions(opts)
>
> env = Environment(
> - options = opts,
> - tools = ['gallium'],
> - toolpath = ['#scons'],
> - ENV = os.environ,
> + options=opts,
> + tools=['gallium'],
> + toolpath=['#scons'],
> + ENV=os.environ,
IMHO, no spaces around = makes harder to read.
> )
>
> # XXX: This creates a many problems as it saves...
> -#opts.Save('config.py', env)
> +# opts.Save('config.py', env)
This is intentional. '# ' comments means text. '#' comments means
commented code.
>
> # Backwards compatability with old target configuration variable
> try:
> @@ -50,10 +50,11 @@ except KeyError:
> pass
> else:
> targets = targets.split(',')
> - print 'scons: warning: targets option is deprecated; pass the targets on their own such as'
> + print 'scons: warning: targets option is deprecated; ' \
> + 'pass the targets on their own such as'
I think that a slightly longer line now and then is fine.
> print
> print ' scons %s' % ' '.join(targets)
> - print
> + print
> COMMAND_LINE_TARGETS.append(targets)
>
>
> @@ -63,30 +64,31 @@ Help(opts.GenerateHelpText(env))
> # Environment setup
>
> with open("VERSION") as f:
> - mesa_version = f.read().strip()
> -env.Append(CPPDEFINES = [
> + mesa_version = f.read().strip()
> +env.Append(CPPDEFINES=[
> ('PACKAGE_VERSION', '\\"%s\\"' % mesa_version),
> - ('PACKAGE_BUGREPORT', '\\"https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_enter-5Fbug.cgi-3Fproduct-3DMesa&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=epgNfkl_mErhe40bZyt56RRGpFVc826xpoi7ePYrj-s&s=XV0oUs1ChJhd-hOXg_12hFGh56gyLObE4mKgqPrEhJA&e= \\"'),
> + ('PACKAGE_BUGREPORT',
> + '\\"https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_enter-5Fbug.cgi-3Fproduct-3DMesa&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=epgNfkl_mErhe40bZyt56RRGpFVc826xpoi7ePYrj-s&s=XV0oUs1ChJhd-hOXg_12hFGh56gyLObE4mKgqPrEhJA&e= \\"'),
> ])
>
> # Includes
> -env.Prepend(CPPPATH = [
> - '#/include',
> +env.Prepend(CPPPATH=[
> + '#/include',
> ])
> -env.Append(CPPPATH = [
> - '#/src/gallium/include',
> - '#/src/gallium/auxiliary',
> - '#/src/gallium/drivers',
> - '#/src/gallium/winsys',
> +env.Append(CPPPATH=[
> + '#/src/gallium/include',
> + '#/src/gallium/auxiliary',
> + '#/src/gallium/drivers',
> + '#/src/gallium/winsys',
> ])
>
> # for debugging
> -#print env.Dump()
> +# print env.Dump()
>
>
> #######################################################################
> -# Invoke host SConscripts
> -#
> +# Invoke host SConscripts
> +#
> # For things that are meant to be run on the native host build machine, instead
> # of the target machine.
> #
> @@ -94,11 +96,11 @@ env.Append(CPPPATH = [
> # Create host environent
> if env['crosscompile'] and not env['embedded']:
> host_env = Environment(
> - options = opts,
> + options=opts,
> # no tool used
> - tools = [],
> - toolpath = ['#scons'],
> - ENV = os.environ,
> + tools=[],
> + toolpath=['#scons'],
> + ENV=os.environ,
> )
>
> # Override options
> @@ -118,8 +120,8 @@ if env['crosscompile'] and not env['embedded']:
>
> SConscript(
> 'src/SConscript',
> - variant_dir = host_env['build_dir'],
> - duplicate = 0, # https://urldefense.proofpoint.com/v2/url?u=http-3A__www.scons.org_doc_0.97_HTML_scons-2Duser_x2261.html&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=epgNfkl_mErhe40bZyt56RRGpFVc826xpoi7ePYrj-s&s=y5C3cEHldMInZpc4sKUN1sdi75BI-Im24ozDHLTVkCM&e=
> + variant_dir=host_env['build_dir'],
> + duplicate=0 # https://urldefense.proofpoint.com/v2/url?u=http-3A__www.scons.org_doc_0.97_HTML_scons-2Duser_x2261.html&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=epgNfkl_mErhe40bZyt56RRGpFVc826xpoi7ePYrj-s&s=y5C3cEHldMInZpc4sKUN1sdi75BI-Im24ozDHLTVkCM&e=
> )
>
> env = target_env
> @@ -133,9 +135,9 @@ Export('env')
> # https://urldefense.proofpoint.com/v2/url?u=http-3A__www.scons.org_wiki_SimultaneousVariantBuilds&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=epgNfkl_mErhe40bZyt56RRGpFVc826xpoi7ePYrj-s&s=AA3ME8-FghoYrACjLh5KUD-87LEq2u3bCsubNEl31As&e=
>
> SConscript(
> - 'src/SConscript',
> - variant_dir = env['build_dir'],
> - duplicate = 0 # https://urldefense.proofpoint.com/v2/url?u=http-3A__www.scons.org_doc_0.97_HTML_scons-2Duser_x2261.html&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=epgNfkl_mErhe40bZyt56RRGpFVc826xpoi7ePYrj-s&s=y5C3cEHldMInZpc4sKUN1sdi75BI-Im24ozDHLTVkCM&e=
> + 'src/SConscript',
> + variant_dir=env['build_dir'],
> + duplicate=0 # https://urldefense.proofpoint.com/v2/url?u=http-3A__www.scons.org_doc_0.97_HTML_scons-2Duser_x2261.html&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=epgNfkl_mErhe40bZyt56RRGpFVc826xpoi7ePYrj-s&s=y5C3cEHldMInZpc4sKUN1sdi75BI-Im24ozDHLTVkCM&e=
> )
>
My impression is that this PEP 8 conformance ends up being a
distraction. These are matters of style, style is something hard to
completely unify -- even Mesa C++ code has multiple code formating
styles --, it doesn't look they will help spot or fix real issues, so I
don't think there's much benefit in PEP 8 conformance.
I think that there's more merit in trying to address Clang static
analyzer / Coverity / etc.
Jose
More information about the mesa-dev
mailing list