Re: [Buildroot] [PATCH 1/2] package/criu: new package

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Marcus Folkesson <marcus.folkesson@gmail.com>
Cc: Julien Olivain <ju.o@free.fr>, buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH 1/2] package/criu: new package
Date: Sat, 9 Sep 2023 15:55:53 +0200	[thread overview]
Message-ID: <20230909155549.15a4b507@windsurf> (raw)
In-Reply-To: <ZPxtEfe6FVIucVWS@gmail.com>

On Sat, 9 Sep 2023 15:03:13 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Thank you for your solid review.
> Apparently I haven't done my homework very well. Sorry for that.

Ah, ah, no worries!

> > Where does this dependency come from?  
> 
> I'm pretty sure that I read that CRIU does not compile with musl
> somewhere, but it seems like it does when I had a closer look.
> I will remove it.

Good.

> > > +	select BR2_PACKAGE_PYTHON_PIP  
> > 
> > It needs pip on the target? Seems odd.  
> 
> What I actually need is host-python-pip as it is used during the
> installation step. Is there a way to only select the host-part from the 
> PYTHON_PIP package?

For most packages, selecting them is useless, and actually most host
packages don't even have a Config.in.host option to select in the first
place. So having host-python-pip in your CRIU_DEPENDENCIES variable is
enough.

However, a package that needs pip during its build process kind of
raises a red flag. Is that package using pip to download/install
additional Python packages during its build procedure? If it's the
case, that's not good, as this downloading of extra stuff by the
package would work around the download logic and legal-info logic of
Buildroot.

> > CRIU_CFLAGS = \
> > 	$(TARGET_CFLAGS) \
> > 	-D__WORDSIZE \
> > 	-D...
> > 
> > However, this is odd. Why aren't those flags set by the package
> > Makefile?  
> 
> I actually took those flags from the yocto recipe [1], but at least
> _GNU_SOURCE seems to be in the package Makefile anyway.
> 
> I will remove __USE_GNU and _GNU-SOURCE.
> Do not know about __WORDSIZE though.

But what do you have it here, then?

> > > +	$(CRIU_MAKE_ENV) $(MAKE) USERCFLAGS="$(CRIU_CFLAGS)" -C $(@D)  
> > 
> > I don't understand how this can know which cross-compiler to use, you
> > are not passing it anywhere as far as I can see.  
> 
> Hrm, I had CROSS_COMPILE=$(TARGET_CROSS) at first, then I though I saw
> that $(TARGET_MAKE_ENV) had it set.

TARGET_MAKE_ENV does not contain CROSS_COMPILE. TARGET_CONFIGURE_OPTS
does, however.

> Unfortunately, my last test was was on x86_64, so I did not notice it
> was wrong.
> 
> I will put it back.

One good thing would be to have a runtime test for criu, in
support/testing. Julien Olivain (in Cc) can probably provide some
guidance here.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot