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

[Dmitry Safonov]

2017-02-21 21:44 GMT+03:00 Kir Kolyshkin <kir at openvz.org>:
> 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.

It would be too complex. And will not make sense if a
project doesn't ship with libcompel's source - which is
only criu at this moment.

If we at some point will separate libcompel repository (again),
then we can very simply drop this dependency.
And for now it does no make any worse for building criu
with internal or external compel, while slightly improving
make dependencies.

For example, at this moment, I've another patch for changing
lds scripts - that's previously sent RFC for read-only .text
section in the parasite. (it still needs some ppc changes,
so I'm keeping it yet).

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

Ok :)

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