Ticket #382 (closed enhancement: fixed)

Opened 9 years ago

Last modified 9 years ago

Respect CC, CXX, CFLAGS, CXXFLAGS and LDFLAGS

Reported by: lilrc Assigned to: jwoithe
Priority: major Milestone:
Component: generic Version: FFADO SVN (trunk)
Keywords: build system, patch Cc:
The device the bug applies to:

Description

Hello developers!

Currently the build system does not honour the common build environment variables CC, CXX, CFLAGS, CXXFLAGS and LDFLAGS. When building ffado in Gentoo this becomes a problem since packages should respect the user's (and package managers) build environment.

The current approach is to have a COMPILE_FLAGS option to add custom compiler flags, but this is not sufficient.

I want to be very clear on this point: I _DO NOT_ ask you to support custom compiler flags, I simply ask you to make your build system respect common build environment variables. It would make maintaining ffado much easier downstream in Gentoo.

To fix this issue I have constructed a patch that does the following with the build system:

  1. Respect CC, CXX, CFLAGS, CXXFLAGS, LDFLAGS.
  2. Notify the user that specifying CFLAGS, CXXFLAGS or LDFLAGS is not supported by you, the ffado-devs, if any of them is specified.
  3. Remove the COMPILE_FLAGS variable as CFLAGS and CXXFLAGS supply the same functionality.
  4. Do not append -O2 from flags if any custom CFLAGS, CXXFLAGS or LDFLAGS are specified.
  5. Make sure oldcf is not overwritten when checking for lrint.

Since the build system is still clear about that custom flags are not supported I think this patch is suitable to apply here (upstream).

Here is the patch against SVN trunk:

Index: SConstruct
===================================================================
--- SConstruct	(revision 2574)
+++ SConstruct	(working copy)
@@ -77,7 +77,6 @@
     EnumVariable('DIST_TARGET', 'Build target for cross compiling packagers', 'auto', allowed_values=('auto', 'i386', 'i686', 'x86_64', 'powerpc', 'powerpc64', 'none' ), ignorecase=2),
     BoolVariable( "ENABLE_OPTIMIZATIONS", "Enable optimizations and the use of processor specific extentions (MMX/SSE/...).", False ),
     BoolVariable( "PEDANTIC", "Enable -Werror and more pedantic options during compile.", False ),
-    ( "COMPILE_FLAGS", "Add additional flags to the environment.\nOnly meant for distributors and gentoo-users who want to over-optimize their built.\n Using this is not supported by the ffado-devs!" ),
     EnumVariable( "ENABLE_SETBUFFERSIZE_API_VER", "Report API version at runtime which includes support for dynamic buffer resizing (requires recent jack).", 'auto', allowed_values=('auto', 'true', 'false', 'force'), ignorecase=2),
 
     )
@@ -87,14 +86,33 @@
 
 env = Environment( tools=['default','scanreplace','pyuic','pyuic4','dbus','doxygen','pkgconfig'], toolpath=['admin'], ENV = buildenv, options=opts )
 
-if env.has_key('COMPILE_FLAGS') and len(env['COMPILE_FLAGS']) > 0:
+# Honour the user choice of compiler (if any).
+if os.environ.has_key('CC') and len(os.environ['CC']) > 0:
+    env['CC'] = os.environ['CC']
+if os.environ.has_key('CXX') and len(os.environ['CXX']) > 0:
+    env['CXX'] = os.environ['CXX']
+
+# Honour the user supplied flags (if any), but notify the user that this
+# is not supported.
+custom_flags = False
+if os.environ.has_key('CFLAGS') and len(os.environ['CFLAGS']) > 0:
+    custom_flags = True
+    env.Append(CFLAGS = str(os.environ['CFLAGS'].replace('\"', '')))
+if os.environ.has_key('CXXFLAGS') and len(os.environ['CXXFLAGS']) > 0:
+    custom_flags = True
+    env.Append(CXXFLAGS = str(os.environ['CXXFLAGS'].replace('\"', '')))
+if os.environ.has_key('LDFLAGS') and len(os.environ['LDFLAGS']) > 0:
+    custom_flags = True
+    env.Append(LINKFLAGS = str(os.environ['LDFLAGS'].replace('\"', '')))
+if custom_flags:
     print '''
  * Usage of additional flags is not supported by the ffado-devs.
  * Use at own risk!
  *
- * Currentl value is '%s'
- ''' % env['COMPILE_FLAGS']
-    env.MergeFlags(env['COMPILE_FLAGS'])
+ * CFLAGS = %s
+ * CXXFLAGS = %s
+ * LINKFLAGS = %s
+''' % (env['CFLAGS'], env['CXXFLAGS'], env['LINKFLAGS'])
 
 Help( """
 For building ffado you can set different options as listed below. You have to
@@ -331,7 +349,7 @@
         oldcf = env['CFLAGS']
     else:
         oldcf = ""
-    oldcf = env.Append(CFLAGS = '-std=c99')
+    env.Append(CFLAGS = '-std=c99')
     if conf.CheckLibWithHeader( "m", "math.h", "c", "lrint(3.2);" ):
         HAVE_LRINT = 1
     else:
@@ -415,7 +433,8 @@
     print "Doing a debug build"
     env.MergeFlags( "-Wall -g -DDEBUG" )
     env['DEBUG_MESSAGES'] = True
-else:
+elif not custom_flags:
+    # Only merge -O2 to flags if the user has not specified custom flags.
     env.MergeFlags( "-O2" )
 
 if env['DEBUG_MESSAGES']:
@@ -769,7 +788,7 @@
 #=== End Revised CXXFLAGS =========================================
 
 
-if needs_fPIC or ( env.has_key('COMPILE_FLAGS') and '-fPIC' in env['COMPILE_FLAGS'] ):
+if needs_fPIC:
     env.MergeFlags( "-fPIC" )
 
 # end of processor-specific section

Thanks in advance,
Karl

Change History

12/01/14 14:45:03 changed by jwoithe

Thank you for your feedback and clear explanation of your patch.

I am not opposed in principle to adding support for CC and friends. At the same time, it is clear that the omission of these was a deliberate choice by the author of the scons scripts and I am curious to know the background to this decision. Since this was written quite some time ago it is possible that this knowledge is no longer around though (Adrian may remember though, since I think he was involved in the scons transition). The rationale may have simply been to make it harder for people to specify custom flags, an activity which we as the developers do not support. In other words, using a non-standard environment variable makes it less likely that someone would inadvertently compile with extra flags. If this was the original intent then I would be inclined to make use of CC etc only if one other additional option or environment variable was supplied to scons. Something like USE_STD_CC_FLAGS for example.

Another alternative approach might be to add a prefix to each of the standard variables: CUSTOM_CC, CUSTOM_CFLAGS, and so on. However, I'm not familiar with your build system so I don't know whether this would solve your issues.

Finally, I would prefer not to remove COMPILE_FLAGS because it is entirely likely that other distributions or users are making use of this. In my view COMPILE_FLAGS should continue to be used if none of the chosen "standard variable" mechanisms are active.

I am sure we can come to some arrangement which works for yourself and others. Please let us know your thoughts about the above issues and we'll see what we can do.

12/01/14 22:33:55 changed by adi

I checked the history. The change was introduced in r1853:

http://subversion.ffado.org/changeset/1853

It also links to #278 for the original discussion.

I wonder why COMPILE_FLAGS isn't enough. At least in Debian, we forward our CFLAGS to this variable and that's it:

http://anonscm.debian.org/cgit/pkg-multimedia/ffado.git/tree/debian/rules#n56

I have no strong feelings. If you guys come up with something new, I'm happy to change the Debian package, especially if ffado's version number changes as well. Downstream follows upstream. ;)

OTOH, USE_STD_CC_FLAGS or even CUSTOM_CC etc. all look good to me. I think the original intent was to have a well-defined build environment that doesn't accidentally change.

12/01/14 22:40:18 changed by jwoithe

Thanks for the info Adrian. I too wonder why COMPILE_FLAGS isn't enough. Perhaps they want the option of plugging into linker with a separate variable. Hopefully Karl will provide feedback over the next day or so based on this additional information.

12/01/14 23:52:34 changed by lilrc

Yes, that is correct. According to Gentoo policy the build systems should respect not just CFLAGS and CXXFLAGS (which COMPILE_FLAGS does fine), but also LDFLAGS since the package manager plugs in --as-needed into it. [1]

I still think having both CFLAGS, CXXFLAGS and COMPILE_FLAGS is not very good (as it is two ways of doing the same thing), but COMPILE_FLAGS could maybe be deprecated so that it can still be used but when it is a deprecation warning can printed redirecting to CFLAGS and CXXFLAGS?

I see one disadvantage with CUSTOM_FLAGS: it does not distinguish between CFLAGS and CXXFLAGS. In Gentoo it would be bad practice to assume that the two are identic (although they often are). This does not matter that much, but there are some C source files in the build system. Separating the two is IMHO a cleaner solution.

I think a boolean RESPECT_CUSOM_ENV option could be introduced (with default to False). If it is set to True the environment will be honoured. This way it is hard to inadvertently pass a bad environment to the build system.

With your considerations in mind I propose an idea for a new patch:

  1. Do not remove COMPILE_FLAGS, but deprecate it and point to CFLAGS and CXXFLAGS.
  2. Introduce RESPECT_CUSTOM_ENV that makes the build system respect the environment if set to True.

The idea with more CUSTOM_* options such as CUSTOM_CC or CUSTOM_CXX is fine too as long as it is possible to set CC, CXX, CFLAGS, CXXFLAGS and LDFLAGS. I do not think it would look good having a number of five CUSTOM_* variables, but if this solution is by you the most preferable I can surely submit a patch for that too.

What do you think?

[1] https://wiki.gentoo.org/wiki/Project:Quality_Assurance/As-needed

12/04/14 12:00:44 changed by lilrc

Here is the patch I described in my previous comment, except CUSTOM_ENV instead of RESPECT_CUSTOM_ENV.

Index: SConstruct
===================================================================
--- SConstruct	(revision 2574)
+++ SConstruct	(working copy)
@@ -77,7 +77,8 @@
     EnumVariable('DIST_TARGET', 'Build target for cross compiling packagers', 'auto', allowed_values=('auto', 'i386', 'i686', 'x86_64', 'powerpc', 'powerpc64', 'none' ), ignorecase=2),
     BoolVariable( "ENABLE_OPTIMIZATIONS", "Enable optimizations and the use of processor specific extentions (MMX/SSE/...).", False ),
     BoolVariable( "PEDANTIC", "Enable -Werror and more pedantic options during compile.", False ),
-    ( "COMPILE_FLAGS", "Add additional flags to the environment.\nOnly meant for distributors and gentoo-users who want to over-optimize their built.\n Using this is not supported by the ffado-devs!" ),
+    BoolVariable( "CUSTOM_ENV", "Respect custom CC, CXX, CFLAGS, CXXFLAGS and LDFLAGS from build environment", False ),
+    ( "COMPILE_FLAGS", "Deprecated. Use the CFLAGS and CXXFLAGS environment variables with CUSTOM_ENV=True instead" ),
     EnumVariable( "ENABLE_SETBUFFERSIZE_API_VER", "Report API version at runtime which includes support for dynamic buffer resizing (requires recent jack).", 'auto', allowed_values=('auto', 'true', 'false', 'force'), ignorecase=2),
 
     )
@@ -87,14 +88,41 @@
 
 env = Environment( tools=['default','scanreplace','pyuic','pyuic4','dbus','doxygen','pkgconfig'], toolpath=['admin'], ENV = buildenv, options=opts )
 
+# Set to True if the user has set any custom flags.
+custom_flags = False
+
 if env.has_key('COMPILE_FLAGS') and len(env['COMPILE_FLAGS']) > 0:
+    print "The COMPILE_FLAGS option is deprecated. Use CFLAGS and CXXFLAGS environment variables with CUSTOM_ENV=True instead"
+    custom_flags = True
+    env.MergeFlags(env['COMPILE_FLAGS'])
+
+if env['CUSTOM_ENV']:
+    # Honour the user choice of compiler (if any).
+    if os.environ.has_key('CC') and len(os.environ['CC']) > 0:
+        env['CC'] = os.environ['CC']
+    if os.environ.has_key('CXX') and len(os.environ['CXX']) > 0:
+        env['CXX'] = os.environ['CXX']
+
+    # Honour the user supplied flags (if any), but notify the user that this is not supported.
+    if os.environ.has_key('CFLAGS') and len(os.environ['CFLAGS']) > 0:
+        custom_flags = True
+        env.Append(CFLAGS = str(os.environ['CFLAGS'].replace('\"', '')))
+    if os.environ.has_key('CXXFLAGS') and len(os.environ['CXXFLAGS']) > 0:
+        custom_flags = True
+        env.Append(CXXFLAGS = str(os.environ['CXXFLAGS'].replace('\"', '')))
+    if os.environ.has_key('LDFLAGS') and len(os.environ['LDFLAGS']) > 0:
+        custom_flags = True
+        env.Append(LINKFLAGS = str(os.environ['LDFLAGS'].replace('\"', '')))
+
+if custom_flags:
     print '''
  * Usage of additional flags is not supported by the ffado-devs.
  * Use at own risk!
  *
- * Currentl value is '%s'
- ''' % env['COMPILE_FLAGS']
-    env.MergeFlags(env['COMPILE_FLAGS'])
+ * CCFLAGS = %s
+ * CXXFLAGS = %s
+ * LINKFLAGS = %s
+''' % (env['CCFLAGS'], env['CXXFLAGS'], env['LINKFLAGS'])
 
 Help( """
 For building ffado you can set different options as listed below. You have to
@@ -331,7 +359,7 @@
         oldcf = env['CFLAGS']
     else:
         oldcf = ""
-    oldcf = env.Append(CFLAGS = '-std=c99')
+    env.Append(CFLAGS = '-std=c99')
     if conf.CheckLibWithHeader( "m", "math.h", "c", "lrint(3.2);" ):
         HAVE_LRINT = 1
     else:
@@ -415,7 +443,8 @@
     print "Doing a debug build"
     env.MergeFlags( "-Wall -g -DDEBUG" )
     env['DEBUG_MESSAGES'] = True
-else:
+elif not custom_flags:
+    # Only merge -O2 to flags if the user has not specified custom flags.
     env.MergeFlags( "-O2" )
 
 if env['DEBUG_MESSAGES']:
@@ -769,7 +798,7 @@
 #=== End Revised CXXFLAGS =========================================
 
 
-if needs_fPIC or ( env.has_key('COMPILE_FLAGS') and '-fPIC' in env['COMPILE_FLAGS'] ):
+if needs_fPIC:
     env.MergeFlags( "-fPIC" )
 
 # end of processor-specific section

12/08/14 04:22:01 changed by jwoithe

  • owner set to jwoithe.

Thanks for this additional patch. I will take a look at both of these in the next few days and merge something along these lines.

12/15/14 04:03:36 changed by jwoithe

A variation of the second patch has been applied to trunk as r2577. Please test and report back if further changes appear necessary. Note that the "oldcf" fix was committed separately (r2576) since it is unrelated to the flag handling change.

12/15/14 05:12:53 changed by lilrc

Thank you very much! I will test it this evening!

12/15/14 10:29:35 changed by lilrc

Works like a charm! Appreciated, thanks!

12/15/14 20:18:59 changed by jwoithe

  • status changed from new to closed.
  • resolution set to fixed.

Lilrc: thanks for confirming.