Re: [UNIKRAFT PATCH v4 2/3] support/build: Add build functions for clang and gcc check

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Vlad,

Thanks for the work. Please find the comment inline.

Thanks & Regards

Sharan

On 2/1/21 4:59 AM, Razvan Deaconescu wrote:
> All good.
>
> Razvan
>
> Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxx> writes:
> > We add have_gcc and have_clang. We modify gcc_version_ge to
> > use have_gcc.
> >
> > Signed-off-by: Alice Suiu <alicesuiu17@xxxxxxxxx>
> > Signed-off-by: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxx>
> > Reviewed-by: Razvan Deaconescu <razvan.deaconescu@xxxxxxxxx>
> > ---
> >   support/build/Makefile.rules | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/support/build/Makefile.rules b/support/build/Makefile.rules
> > index 2a8a294..c201bc4 100644
> > --- a/support/build/Makefile.rules
> > +++ b/support/build/Makefile.rules
> > @@ -29,10 +29,20 @@ lc = $(subst A,a,$(subst B,b,$(subst C,c,$(subst 
> > D,d,$(subst E,e,$(subst F,f,$(s
> >   # upper case
> >   uc = $(subst a,A,$(subst b,B,$(subst c,C,$(subst d,D,$(subst
> > e,E,$(subst f,F,$(subst g,G,$(subst h,H,$(subst i,I,$(subst
> > j,J,$(subst k,K,$(subst l,L,$(subst m,M,$(subst n,N,$(subst
> > o,O,$(subst p,P,$(subst q,Q,$(subst r,R,$(subst s,S,$(subst
> > t,T,$(subst u,U,$(subst v,V,$(subst w,W,$(subst x,X,$(subst
> > y,Y,$(subst z,Z,$(1)))))))))))))))))))))))))))
> >   
> > +# test if GCC is set as a compiler
> > +define have_gcc =
> > +$(shell if [ $(CC) = gcc ] ; then echo y ; else echo n ; fi)
shouldn't this check also for check for $(CONFIG_CROSSCOMPILE)gcc, 
wouldn't it return false when cross-compiling with gcc?
We can fix it within make using the this syntax instead of calling shell:

$(if $(filter %gcc,$(CC)),y,n)

> > +endef
> > +
> > +# test if CLANG is set as a compiler
> > +define have_clang =
> > +$(shell if [ $(CC) = clang ] ; then echo y ; else echo n ; fi)
> > +endef
> > +
> >   # test whether GCC version is greater than or equal to the minimum requirement
> >   # gcc_version_ge $gcc_major,$gcc_minor
> >   define gcc_version_ge =
> > -$(shell if [ $(CC_VER_MAJOR) -gt $(1) ] || ([ $(CC_VER_MAJOR) -eq $(1) ] && [ 
> > $(CC_VER_MINOR) -ge $(2) ]) ; then echo y ; fi)
> > +$(shell if [ $(call have_gcc) = y ] ; then if [ $(CC_VER_MAJOR) -gt $(1) ] || ([ 
> > $(CC_VER_MAJOR) -eq $(1) ] && [ $(CC_VER_MINOR) -ge $(2) ]) ; then echo y ; fi 
> > ; else echo n ; fi)
> >   endef
You are changing the behavior of this definition. The definition is 
expected to return y or empty string. If you `echo n` here, the caller 
has to checked explicitly check if the value returned is y. Typically 
only not empty string check would be performed. So if you echo n it will 
result if clause evaluating true on both account.
> >   
> >   # print error and stop build when GCC version doesn't meet the minimum requirement