Re: [CrystalSpace] #965: CS-2.0_beta3: Build system not fit for system wide installation on Linux

#965: CS-2.0_beta3: Build system not fit for system wide installation on Linux
--------------------------+-------------------------------------------------
 Reporter:  yamakuzure    |       Owner:  admin           
     Type:  defect        |      Status:  new             
 Priority:  major         |   Milestone:  Version 2.0 pre1
Component:  build system  |     Version:  V2.0            
 Keywords:                |  
--------------------------+-------------------------------------------------

Comment(by sunshine):

 Replying to [ticket:965 yamakuzure]:
 > [...] configure allows to set specific paths for the parts of the
 installation and to add custom `CFLAGS`, `CXXFLAGS` and `LDFLAGS`. The
 current build system does not take care of those custom flags, which ends
 in `ftjam` segfaulting if the command lines become too large (over ~1,000
 characters). The first thing that crashes is `config.status` when trying
 to build the `Makefile` emulation.

 It would be helpful to present the actual command-line composed by Jam
 which is greater than 1000 characters so that we can determine if the
 generated command is bogus. (This has happened in the past when some
 change was made to the CS `Jamfiles` resulting in unnecessarily long
 commands.) Without seeing the actual problem command(s), it also is
 difficult to judge the involvement of `CFLAGS`, `CXXFLAGS`, and `LDFLAGS`.

 Also have you tried building with stock `jam`, rather than `ftjam` which
 we do not support explicitly? You might also have better luck with the
 `jam` bundled with CS. If I recall correctly, neither stock `jam` nor the
 one bundled with CS suffer from the 1000-character limit. The `jam`
 bundled with CS should be available automatically as `./jam` in the CS
 build directory if `jam` was not found on your system.

 > '''1'''.: `configure.ac` forces `/usr/local/lib` (and
 `/usr/local/include` if present) upon users, which is a bad thing to do.
 Patch in a check to not do this if the `--prefix`, `--libdir` and/or
 `--includedir` options lead to default paths.

 It is not at all clear what the intention of this patch is, nor what you
 mean with ''is a bad thing to do''. Please provide more context explaining
 what actually goes wrong in your case, such as error messages or a
 concrete argument if the issue is a philosophical one.

 With this patch, it appears that you are conflating locations which CS
 searches for 3rd-party libraries with locations into which CS gets
 installed. The code which this patch touches has nothing to do with
 locations into which CS gets installed. This code is merely a convenience
 to help `configure` locate 3rd-party libraries on platforms on which the
 development tools do not search `/usr/local/{include,lib}` by default.
 Options `--prefix`, `--libdir`, `--includedir`, on the other hand, are all
 installation-time options which the CS build system does respect.

 Minor comment: The patch also makes meaningless whitespace changes (adding
 blank line and converting tab to spaces) which make it more difficult to
 review.

 > '''2'''.: The three modes 'optimize', 'profile' and 'debug' clash
 horribly with user set `FLAGS`. On Gentoo system wide build flags are used
 and are added up to insanely long command lines with the presets that
 easily consists of a lot of superfluous flags. These two patches add a new
 mode 'custom' that takes those custom flags and adds only strictly
 mandatory flags.

 Adding a new mode which falls back to the system-wide build settings seems
 reasonable, however, the name ''custom'' may be a bit too generic since it
 doesn't necessarily convey any particular meaning, although I can't think
 of a better name. Possibilities include ''fallthru'', ''inherit'',
 ''env'', ''user'', ''system'', ''global'', though they may not be any
 better than ''custom''.

 > ''Patches'': `crystalspace-2.0-02-add_custom_mode.patch`

 There seems to be some whitespace problem in the first hunk of this patch
 where the description of `--enable-cpu-specific-optimizations` has been
 re-wrapped.

 The help text for the new ''custom'' option is somewhat misleading. Rather
 than saying that it builds "without special `CFLAGS`", it might make more
 sense to say that it employs those already available in the environment.

 Finally, the patch forgets to update the comment starting "Determine build
 mode" to also mention ''custom''.

 > +AS_IF([test $enable_cpu_specific_optimizations != no -a
 "$cs_build_mode$" = !"custom"],

 It would be cleaner to leave this conditional alone, and to instead
 separately set `enable_cpu_specific_optimizations` to `no` if build mode
 is ''custom''.

 > '''3'''.: Before the new custom mode can be set into action some new
 functions to `mk/autoconf/trim.m4` have to be introduced. This patch does
 that.

 Unfortunately, these additional functions both lack robustness and will
 break the build for other platforms. Problems with
 `CS_{APPEND,PREPEND}_TRIMMED` include:

  - The comments state that these functions will remove all duplicate
 items. For instance, "`-foo -bar -foo -baz -bar`" will be collapsed to
 "`-foo -bar -baz`". Unfortunately, this will break cases in which build
 flags are not atomic. For instance, real-world cases include build flags
 "`-framework AppKit -framework Foundation -arch x86_64 -arch i386`" which
 will be collapsed to the bogus "`-framework AppKit Foundation -arch x86_64
 i386`". Furthermore, there have been ugly cases of circular references
 between libraries, such that "-lfoo -lbar -lfoo" was needed to make a
 build successful, so collapsing it to "-lfoo -lbar" could cause breakage.

  - The test for a duplicate flag is flawed. For instance, "`-foobar -foo
 -bar`", will be collapsed incorrectly to "`-foobar`" simply because `foo`
 and `bar` appear textually in `foobar` despite the fact that they are
 actually distinct flags.

  - Use of "`echo -n`" is unportable. As an alternative, you could use
 Autoconf's `AS_ECHO_N` macro instead.

  - The purpose of the `$append_text` variable is not clear. It appears to
 mirror the value of the passed in shell variable at all times, so why not
 just use the shell variable instead?

 In the `CS_ERASE_VAR` function why test if the variable is not empty
 before setting it to empty? Simply setting it to an empty string
 unconditionally would be clearer. Such an operation also does not
 necessarily warrant a separate function, so `CS_ERASE_VAR` may be
 overkill.

 (A minor side note: In the world of Autoconf, unnecessarily assigning a
 value to a variable -- even an empty value -- changes how Autoconf treats
 that variable. For `CFLAGS` and its cousins, doing so suppresses
 Autoconf's behavior of automatically adding certain default build flags to
 the variables. In the particular case of CS, this isn't an issue, but it's
 worth noting for the general case.)

 > '''4'''.: These patches substitute the `emitter` calls from
 `configure.ac` with the new function introduced in '''3''' to save flags
 in specific shell vars instead of pumping them unchecked into `Jamconfig`.

 This approach is pretty much flawed from the get-go for the reasons given
 above, particularly where non-atomic flags are concerned (such as
 `-framework` and `-arch`). If shorter command-lines are indeed needed due
 to broken tools such as `ftjam`, then it would be better and easier to
 fold out duplicates within the Jam system itself. In fact, it already does
 this in several cases, and it does so intelligently, without botching non-
 atomic flags. In particular, see `InferFlagTuples` and `RemoveDupFlags` in
 `mk/jam/flags.jam`, as well as the callers of those functions. If you find
 that the current attempts by the Jam system are not aggressive enough at
 folding out duplicates, then it might make sense to identify specific
 places where it is too timid, and focus on resolving those.

 > ''Patches'':
 `crystalspace-2.0-05-remove_emit_from_configure_ac_01.patch`

 Overall, the wholesale replacement of `CS_EMIT_BUILD_FLAGS` invocations
 with `CS_APPEND_TRIMMED` has the drawback of coupling individual
 `configure` checks more tightly than they ought to be, whereas they
 presently are mostly self-contained. Being self-contained is beneficial
 since it means that other projects (CS-related or not) can copy and paste
 checks into their own `configure` scripts easily. Moreover, this wholesale
 replacement does not address the problem of duplicate flags introduced by
 any of the helper checks in `mk/autoconf/.*m4`, so it feels more like a
 temporary bandage than a proper fix.

 The hunk which unconditionally clears `CFLAGS` and its cousins just seems
 wrong. These flags are not only emitted into `Jamconfig`, but they are
 used by `configure` itself when performing checks, and the user may very
 well have assigned specific values to these flags before running
 `configure` in order to alter how the checks are performed or to make
 checks succeed which might otherwise fail. Therefore, clearing them
 unconditionally is the wrong approach; and it's not even clear what
 benefit you gain by doing so.

 This patch also seems to mix several other changes unrelated to the
 described purpose. For instance, without explanation, it changes
 backquotes to forward quotes in comments and printed messages. If the
 backquotes were causing some sort problem for your version of Autoconf or
 within your editor, then you should explain so and submit these fix-ups as
 distinct patches in order to make the review process simpler.

 Furthermore, the replacement of backquote with forward quote was not
 performed carefully and actually breaks a Cygwin-related test involving
 the expression "`uname | grep -i CYGWIN`" within `AS_IF`.

 > `crystalspace-2.0-06-remove_emit_from_configure_ac_02.patch`,
 `crystalspace-2.0-07-remove_emit_from_configure_ac_03.patch`,
 `crystalspace-2.0-08-remove_emit_from_configure_ac_04.patch`,
 `crystalspace-2.0-09-remove_emit_from_configure_ac_05.patch`

 I don't really have much more to say about the rest of the patches which
 touch `configure.ac` since they all suffer from the same problems noted
 above. Moreover, there does not seem to be any coherent organization to
 these patches which would help a reviewer understand the relationship
 between them or the order of changes. Mostly, they appear to be moving
 text around and arbitrarily replacing `CS_EMIT_BUILD_FLAGS` with
 `CS_APPEND_TRIMMED`, but it is not at all clear why they are split up as
 they are. The impression is that some changes were made toward getting the
 project to build and a patch was created, then some more changes were made
 and another patch, then more changes, etc. Without any real organization
 which would make sense to someone trying to review the code, the
 progression of patches seems random.

 > '''5'''.: This patch does the same as the five in '''4''', but with
 `mk/autoconf/compiler.m4`.

 All of the files in `CS/mk/autoconf` are intended to be project-agnostic,
 which means that they can be used in projects other than CS, so the
 modifications to `compiler.m4`, in particular, are very problematic. Any
 project external to CS which is based upon the CS build system (via
 `CS/scripts/jamtemplate`, for instance) will be broken by this change.

 > '''8'''.: `cs-config` doesn't look into `/usr/lib[32|64]`, yet.

 Unfortunately, this patch performs a wholesale conversion of tabs to
 spaces, which makes it appear as if the entire file was rewritten, thus
 completely obscuring from the person reviewing the patch any ''real''
 changes made to support `/usr/lib{32,64}`. Even with the whitespace
 changes removed, the patch is plagued by several instances of churn, such
 as splitting or joining of lines of code, unrelated to the fundamental
 `/usr/lib{32,64}` change, once again making the patch more difficult to
 review. (Churn which makes code formatting more consistent may be okay,
 but should be submitted as a separate patch so as to not obscure ''real''
 changes in a more fundamental patch.)

 Regarding the ''real'' change in this patch, if Frank or another developer
 also reviews it and doesn't find any issues with it, it's probably okay to
 commit (but only the actual part of the patch which adds `/usr/lib{32,64}`
 support -- not any of the other churn).

-- 
Ticket URL: <http://www.crystalspace3d.org/trac/CS/ticket/965#comment:4>
CrystalSpace <http://www.crystalspace3d.org/>
Free open-source 3D SDK.
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/