[OpenWrt-Devel] [PATCH 01/13] build metadata: Allow to build a subset of profiles in a single build

[Daniel Dickinson]

On 19/01/16 05:04 AM, Felix Fietkau wrote:

[snip]

[legacy UBI_OPTS targets]

>>
> Please don't add extra abstractions to this code. The
> $(PROFILE)_UBI_OPTS stuff is legacy crap and any target still using it
> should just stay that way and not get multi-profile selection until it
> is converted to the new code.

Okay.

>> +# $(1) macro
>> +# $(2) macro parameter
>> +define BuildForProfiles
>> +  $(foreach profile,$(PROFILES_BUILD),$(call $(1),$(2),$(profile)))
>> +endef
> Please don't call all the legacy Image/* code once per profile.
> This will generate unnecessary duplicate files and extra build time.
> If you rework targets to just stop using $(PROFILE) and check the
> profile enabled/disabled variables directly, you don't need this at all.

I think doing that, particularly on targets such as ar71xx, turns this 
into a much broader reworking of profile/image system than I had planned 
on undertaking, or thought belonged in a single patch series.  See below.

>> diff --git a/include/target.mk b/include/target.mk
>> index f129298..92e52db 100644
>> --- a/include/target.mk
>> +++ b/include/target.mk
>> @@ -70,6 +70,11 @@ define Profile
>>     DUMPINFO += \
>>   	echo "Target-Profile: $(1)"; \
>>   	echo "Target-Profile-Name: $(NAME)"; \
>> +	echo "Target-Profile-Type: $(if $(filter All,$(1)),all,$(if $(filter Minimal,$(1)),minimal,normal))"; \
>> +	echo "Target-Profile-Skip: $(if $(filter Default,$(1)),$(if $(PROFILE_SKIP_DEFAULT),1,0),$(if $(PROFILE_SKIP),1,0))"; \
>> +	echo "Target-Profile-Skip-Single: $(if $(filter All,$(1)),$(if $(PROFILE_SKIP_SINGLE),1,0),0)"; \
>> +	echo "Target-Profile-Default: $(if $(filter All,$(1)),1,$(if $(filter Default,$(1)),1,0))"; \
>> +	echo "Target-Profile-Build-If-All: $(if $(filter All,$(1)),0,$(if $(filter Minimal,$(1)),0,1))"; \
> Please get rid of the magic flag setting based on the profile name. Make
> these explicit by setting variables in the appropriate profiles.

Ok.

> Also, the types and flags are rather confusing here. From the metadata
> processing point of view, there should be nothing special about profiles
> named 'Default' and 'Minimal' aside from a few simple distinctions:
> There should be one profile that is enable by default
> There should be a distinction between device profiles and profiles that
> build all (or most) devices.
> If you write the code to clearly handle the above, this should make the
> rest of your patch less confusing.
>
> Now that I read your patches again, I get the sense that I disagree with
> part of your changes on a conceptual level. I think giving the
> impression that selecting multiple entries in the profile list will
> 'Build multiple profiles' seems wrong to me, making the description
> 'Build all profiles' even more wrong. The code is not doing that,
> because that would imply having a different package selection for every
> profile build, and the build system can't do that.
>
> I think a cleaner description would be building for all devices or a
> subset of the devices on the list, and just not feed the impression that
> it has that much to do with 'profiles' at all.
>
> Essentially, this is nothing but a simple way to make an image that is
> compatible with a specific set of devices, and we should present it to
> the user that way.

I the confusion stems from that fact that in the past selecting a 
profile was how you picked what images to build, and so I thought of a 
profile as an 'image-selector' rather as in terms of the package sets 
the profiles select.

In fact as I pointed out in another email the profiles on the targets 
that have 'Default' or 'Generic' targets that build images for all 
profiles for the arch_subtarget end up all using the same rootfs, but do 
still generate distinct images, or in the case of brcm47xx one has a few 
sets of generic profiles that build multiple images each.

There are actually other moving parts to the 'profile/image' equation 
(which is my on ar71xx most profiles correspond to a single device's 
images), which include the per-image kernel and bootloader 'magic' (and 
I think if the correct terminology is used, the kernel and 'magic' to 
get images accepted by the bootloader is a per-image (or image family) 
piece rather than a per-profile piece.

There perhaps should really be an additional tier in the build selection 
so that instead of only talking about selecting profiles, one selects 
from generic profiles and then makes a sub-selection of images to build 
for a particular profile, and to get rid of the number of board-specific 
'profiles' that are each the same as the other except they invoke the 
magic for a specific board.

Some care needs to be take with platforms such as sunxi because of e.g. 
board-specific u-boot.  There is no reason the uboots can't all be 
built, but one probably doesn't want to build uboot for boards one is 
not using.

Regards,

Daniel
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel