[systemd-devel] Fw: [PATCH 1/2] systemd-helpers

[Carlos Morata Castillo]

Hi,

El Sat, 10 Jan 2015 14:28:26 -0500
Dave Reisner escribió:

>On Sat, Jan 10, 2015 at 01:33:05PM +0100, Carlos Morata Castillo wrote:
>> Hi,
>> 
>> As stated here, we should use a library for bash autocompletions (maybe even with include guards).
>> http://cgit.freedesktop.org/systemd/systemd/commit/shell-completion/bash/localectl?id=a72d698d0d9ff9c158155b44cdc77376df31a317
>> 
>> Explanation:
>> 
>> Using autotools make the autocompletions to /usr/share/bash-completions/completions (as the exit from pkg-config --variable=completionsdir
>> bash-completion).
>> We ended up having multiple function definitions and even messing around with the global bash function namespace, as the functions are called
>> __get_something being possible to redefine other binary bash function. 
>
>Why the exception for busctl and systemctl?
>

For two reasons:
1-busctl because is using busctl itself for every completion and being systemd modular maybe the actual host doesn't 
have it installed, failed then the autocompletion. 
2-systemctl.in. I just left out this file until further review of this approach.

>You aren't actually solving any problems with redefinition in this
>patch. You're actually making the problem worse. Instead of redefining
>*some* common functions on completion load, you're forcing *all* common
>functions in systemd-helpers to be redefined (even if only 1 is used). I
>don't really see this as a problem, though, and you don't explain why
>you seem to think it is.
>

That's why I mentioned the "include guards" or how you said m4_include would do the job way better.
The purpose is to make a library, i.e:
1-Standarize and don't duplicate code
2-Later on you realize that you need to extend some function, e.g: get_machines to work differently 
 inside a container or in a host. You just have to change one function not actually each get_machines definition. 

Both, main cases for using a library. :)


>How is sourcing this library supposed to work? Each file contains:
>
>  #Common functions
>  . ./systemd-helpers
>
>But that file won't be in $PWD when completions are loaded. Sourcing
>works like any other command -- relative paths will be relative to the
>current process's $PWD, not relative to the disk file calling source.
>Without cd'ing into /usr/share/bash-completions/completions, this
>doesn't work (and please don't cd from completions).
>

You're totally right.
I fixed it.

----------
>cat a
#!/bin/bash

function a(){
    echo Doing a;
   }

>cat hi 
#!/bin/bash

. ${BASH_SOURCE[0]%/*}/a 

function _do_a(){
   a
 }

complete -F _do_a hi

>cp a hi /usr/share/bash-completion/completions/
>touch hi
>hi Doing a
Doing a
Doing a
Doing a
>complete -p hi
complete -F _do_a hi

>I'm not against deduplicating this common code, but you'd be better off
>using m4_include to preprocess the completions at build time.
>
>Cheers,
>dR

I agree with you. :)



Cheers.