[apparmor] [RFC] [patch] Makefile automatically fallback to USE_SYSTEM=1

[John Johansen]

On 03/12/2014 04:45 PM, Christian Boltz wrote:
> Hello,
> 
> Am Dienstag, 11. März 2014 schrieb John Johansen:
>> On 03/09/2014 09:36 AM, Christian Boltz wrote:
>>> Am Freitag, 7. März 2014 schrieb John Johansen:
>>>> So this is a patch I have been using for a while to get around
>>>> having
>>>> to specify USE_SYSTEM=1 every time I run the tests. Is it worth
>>>> upstreaming and applying to the other Makefiles that currently can
>>>> take USE_SYSTEM as a parameter
>>>>
>>>> If USE_SYSTEM=1 is NOT specified
>>>>
>>>>   it tries to find a local build and if it can't find one, it sets
>>>>   USE_SYSTEM=1 and issues a warning message
>>>>
>>>> If USE_SYSTEM=1 is specified
>>>>
>>>>   it only tries building against the system libraries
>>>
>>> I understand why you like this behaviour, but I don't like it too
>>> much because it introduces some "surprising" behaviour. (And I
>>> doubt someone reads the warning if the test succeeds ;-)
>>
>> I'm not sure why its a surprising behavior, but okay. 
> 
> Maybe I should reprase "surprising" to "behaviour depends on other parts 
> of the source tree _and_ on system-wide installed binaries". 
> 
>> Question is who
>> do you see the consumer as?
> 
> Basically everybody who builds AppArmor - which can mean a developer or 
> packager (who does or does not know about USE_SYSTEM), and also any 
> build service (which for sure does not know about USE_SYSTEM).
> 
> I'm OK if the result varies between "works" and "fails with an error 
> message" - that's a very obvious difference, and everybody (including 
> OBS etc.) will notice it. However I don't like the idea of "somehow 
> works, but in a totally different way" because that might give 
> interesting[tm] results.
> 
>>> Thinking about it - can you implement an additional condition like
>>> "file common/enable-auto-fallback-to-USE_SYSTEM exists" (or an
>>> environment variable you set in .bashrc, but a file sounds better)?
>>>
>>> That would mean people have to actively opt-in to the automatical
>>> fallback by touch'ing the file once in their bzr checkout. This
>>> would
>>> fix the "surprise" part.
>>>
>>> With the opt-in file added, the patch looks good to me.
>>
>> using a file is possible, but how about opt-out instead? It would be
>> easy to add USE_SYSTEM=0, to opt-out. So if USE_SYSTEM is defined
>> (either 0 or 1) you get one or the other behavior other wise it tries
>> to build against one then the other
> 
> That would mean everybody who builds AppArmor packages needs to add 
> USE_SYSTEM=0 in the spec/whatever to be on the safe side, and we also 
> need to explain it in README.
> 
> Using an opt-in file is better - it gives packagers etc. a guaranteed 
> consistent behaviour, and allows you to enable the automatic fallback by 
> creating a file [1]. (If I had to guess, I'd say only AppArmor 
> developers are interested in the fallback - everybody else most probably 
likely

> prefers the consistent behaviour without automatic surprises.)
> 
heh, I can live with having to touch a file to get different behavior
what is really annoying is having to specify USE_SYSTEM all the time

> BTW: There's only an "ifdef USE_SYSTEM" check, but no check for the 
> actual value. Did you test if USE_SYSTEM=0 does what you expect? ;-) 
> (I seriously doubt after giving it a short test...)
> 
oh no, the ifdef would have to be reworked to an ifeq or ifneq. eg.
  ifeq($(USE_SYSTEM), 0)