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

[Dmitry Safonov]

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

>
>
>>
>>>          $(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 $@
>>
>>
>

-- 
             Dmitry