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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 27/02/2023 14:54, Jan Beulich wrote:
> 
> 
> On 27.02.2023 14:41, Michal Orzel wrote:
>> 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 understand that the changes would be more involved, but I disagree with
> your "two parts" statement: If what I've outlined was already the case,
> your patch would not even exist (because crypto/ would already be taken
> care of wherever needed).
> 
>> 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 )
> 
> The fact that, afaict, this won't do is related to the outline I gave.
> You've referred yourself to need-config. Most if not all of the goals
> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets.
> Hence your change above is effectively a no-op, because CONFIG_CRYPTO
> will simply be unset in the common case. Therefore if an easy change is
> wanted, the original version of it would be it. An intermediate
> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86.
> But neither would preclude the same issue from being introduced again,
> when a new subdir appears.
I understand your concerns. However, I cannot see how your suggested changes
e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue.
They would reduce the repetitions (hence I called them cleanup/improvements)
but as we want to keep tags,cscope,clean as no-dot-config targets, we would 
still not be able to use:
SUBDIRS-$(CONFIG_CRYPTO) += crypto
and use it in all_sources (because as you pointed out, it will be a no-op).

Also, why do we care so much about guarding crypto/ if we take into account
so many architecture/config dependent files for tags/cscope (e.g. in drivers, 
lib/x86, etc.)?
If these are no-dot-config targets, then we should take everything, apart from 
really big directories
like arch/ ones.

~Michal