Re: [PATCH] build: add crypto/ to SUBDIRS

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Jan,

On 27/02/2023 11:10, Jan Beulich wrote:
> 
> 
> On 27.02.2023 10:53, Michal Orzel wrote:
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE
>>       $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>>       $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
>> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>>
>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test
>>  define all_sources
>>      ( find include -type f -name '*.h' -print; \
>>        find $(SUBDIRS) -type f -name '*.[chS]' -print )
> 
> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo
> also only be included when selected (or at the very least only when an
> arch might select it, which afaics is possible on x86 only right now).
> 
> It would also help if in the description you made explicit that SUBDIRS
> isn't used for anything else (the name, after all, is pretty generic).
> Which actually points at an issue: I suppose the variable would actually
> better be used elsewhere as well, e.g. in the _clean: rule and perhaps
> also in the setting of ALL_OBJS-y. (That'll require splitting the
> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and
> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which
> has caused crypto to be missing from SUBDIRS.
> 
I think what you wrote can be split into 2 parts: the part being a goal of this 
patch
and the cleanup/improvements that would be beneficial but not related to this 
patch.
The second part involves more code and there are parts to be discussed:

1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to carve 
out test/ dir
that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the order 
of ALL_OBJS-y matters
for linking, so we would need to transfer the responsibility to SUBDIRS. The 
natural placement of
SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. However, 
when doing clean (next point),
need-config is set to n and SUBDIRS would be empty. This means it would need to 
be defined somewhere at the
top of the Makefile (thus harder to make sure the linking order is correct).

2) If we deicide to use SUBDIRS for _clean rule, then we would need some 
foreach loop, right?
Apart from that, there are other directories that are not part of SUBDIRS i.e. 
include/, tools/.
Together with SUBDIRS variable, it would create confusion (these dirs are also 
sub-directories, so why
not having them listed in this variable?). Also, I can see that we do clean not 
only for $(TARGET_ARCH)
but for all the existing architectures.

I would prefer to stick for now to the goal of this patch which is to add 
crypto/ so that it is taken
into account for the tags/csope generation. Would the following change be ok 
for that purpose?

diff --git a/xen/Makefile b/xen/Makefile
index 2d55bb9401f4..05bf301bd7ab 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE
        $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
        $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
'ALL_LIBS=$(ALL_LIBS-y)' $@
 
-SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
+SUBDIRS-$(CONFIG_CRYPTO) += crypto
+SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y)
+
 define all_sources
     ( find include -type f -name '*.h' -print; \
       find $(SUBDIRS) -type f -name '*.[chS]' -print )


~Michal