[CRIU] [PATCH 1/6] criu/pie/Makefile: rm old compel leftovers

[Kir Kolyshkin]

On 02/20/2017 03:08 AM, Dmitry Safonov wrote:
> 2017-02-20 13:28 GMT+03:00 Kir Kolyshkin <kir at virtuozzo.com>:
>> On 02/20/2017 02:04 AM, Dmitry Safonov wrote:
>>> 2017-02-20 6:13 GMT+03:00 Kir Kolyshkin <kir at openvz.org>:
>>>> Since we treat compel as an external tool, and since compel can now
>>>> provide ldflags, plugins and so on, there is no need to have this
>>>> extra code in Makefile.
>>>>
>>>> Note that because of the changes, parasite/restorer is no longer rebuilt
>>>> when there are changes in compel. To me, this fits well to a vision that
>>>> compel is an external thing. This means, unfortunately, that if you have
>>>> changed compel code, you need to run 'make clean' for a proper build.
>>> Hmm, not sure about this change.
>>> What's the point about building criu with external compel binary?
>>
>>  From my POV, compel will ultimately be a separate tool/library,
>> maybe even hosted in a separate repository. With compel CLI tool,
>> it is now possible to compile criu/pie either with installed or uninstalled
>> compel (I think I even tested that some time ago). We are untying
>> compel from criu, and this includes these intra-dependencies.
>>
>>
>>>> This also makes it possible to use e.g. installed compel instance.
>>>>
>>>> Signed-off-by: Kir Kolyshkin <kir at openvz.org>
>>>> ---
>>>>    criu/pie/Makefile | 13 +++----------
>>>>    1 file changed, 3 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/criu/pie/Makefile b/criu/pie/Makefile
>>>> index f6e9ff2..68a2f4b 100644
>>>> --- a/criu/pie/Makefile
>>>> +++ b/criu/pie/Makefile
>>>> @@ -1,8 +1,5 @@
>>>>    target         += parasite restorer
>>>>
>>>> -parasite-obj-e += ./compel/plugins/std.built-in.o
>>>> -restorer-obj-e += ./compel/plugins/std.built-in.o
>>>> -
>>>>    parasite-obj-y += parasite.o
>>>>    restorer-obj-y += restorer.o
>>>>    restorer-obj-y += ./$(ARCH_DIR)/restorer.o
>>>> @@ -33,7 +30,6 @@ ccflags-y     += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0
>>>>
>>>>    ifneq ($(filter-out clean mrproper,$(MAKECMDGOALS)),)
>>>>            CFLAGS += $(shell $(COMPEL_BIN) cflags)
>>>> -       compel_std      := $(shell $(COMPEL_BIN) plugins)
>>>>    endif
>>>>
>>>>    ifeq ($(SRCARCH),arm)
>>>> @@ -43,19 +39,16 @@ endif
>>>>    asflags-y      += -D__ASSEMBLY__
>>>>
>>>>    BLOBS          += $(obj)/restorer-blob.h $(obj)/parasite-blob.h
>>>> -LDS            :=
>>>> $(SRC_DIR)/compel/arch/$(SRCARCH)/scripts/compel-pack.lds.S
>>>>
>>>>    .SECONDARY:
>>>>
>>>>    target-name = $(patsubst criu/pie/%-blob.h,%,$(1))
>>>>
>>>> -$(obj)/restorer.built-in.o: $(compel_std)
>>>> -$(obj)/parasite.built-in.o: $(compel_std)
>>>> -$(obj)/%.built-in.bin.o: $(obj)/%.built-in.o $(obj)/pie.lib.a $(LDS)
>>>> +$(obj)/%.built-in.bin.o: $(obj)/%.built-in.o $(obj)/pie.lib.a
>>> If you're building with external compel, this dependency does
>>> not make you rebuild builtins until you touch lds script.
>>> I do not see anything wrong with this.
>>
>> 1. With external compel, we don't really know where the linker script lives,
>> or is there a linker script at all -- we just ask compel to provide ldflags.
>>
>> 2. With any external tool, its dependencies are not added
>> (for example, you do not add a dependency upon /usr/bin/gcc etc).
> Yeah, but for a while, until it's in the same repository - it makes
> sense to keep this dependency IMO.
>
>> 3. I don't see anyone ever changing these linker scripts since they were
>> added.
> That's because the patch which moved them from criu was formatted
> without `-M' option.
> Look at those hashes from master:
> e437e616af29d 81f7f7038643f 18db781a8e6cd 8d8d638454ab3
> 303b875892f3d b8d0d5e779ac3 f1d7964e268e7

Thanks! OK, maybe it makes sense to keep it.

What I can think of is something like this:

# extract the -T argument from compel ldflags
LDS    := $(shell $(COMPEL_BIN) | sed 
's/.*[[:space:]]-T[[:space:]]\([^[:space:]]*\).*$/\1/'

Or, we can introduce compel lds command.

Or, leave it as is for now. That's what I'll do :)

>
>>
>>>>           $(call msg-gen, $@)
>>>> -       $(Q) $(LD) $(shell $(COMPEL_BIN) ldflags) -o $@ $^
>>>> +       $(Q) $(LD) $(shell $(COMPEL_BIN) ldflags) -o $@ $^ $(shell
>>>> $(COMPEL_BIN) plugins)
>>>>
>>>> -$(obj)/%-blob.h: $(obj)/%.built-in.bin.o
>>>> $(SRC_DIR)/compel/compel-host-bin
>>>> +$(obj)/%-blob.h: $(obj)/%.built-in.bin.o
>>> It may make sense, but then why not to change dependency
>>> `$(SRC_DIR)/compel/compel-host-bin' to `$(COMPEL_BIN)'?
>>
>> I believe this dep was added to make sure compel binary (compel-host-bin) is
>> built before
>> make executes this target. As we have this dependency in top level Makefile,
>> it's not really
>> needed here.
> Ok
>
>>
>>>>           $(call msg-gen, $@)
>>>>           $(Q) $(COMPEL_BIN) hgen -f $< -o $@
>>>