Buildroot Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Guillaume Chaye <guillaume.chaye@zeetim.com>
Cc: buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCHv3 1/1] package/heimdal: Upgrade existing package and add target package.
Date: Mon, 21 Apr 2025 23:49:00 +0200	[thread overview]
Message-ID: <20250421234900.59e02be8@windsurf> (raw)
In-Reply-To: <20250214124520.250825-1-guillaume.chaye@zeetim.com>

Hello Guillaume,

On Fri, 14 Feb 2025 07:45:20 -0500
Guillaume Chaye <guillaume.chaye@zeetim.com> wrote:

> Signed-off-by: Guillaume Chaye <guillaume.chaye@zeetim.com>

Your patch is doing *tons* of things, and the commit log is empty. This
just cannot work, as it means reviewers/maintainers cannot understand
why you're doing all those changes. Why are you bringing all those
additional patches?


> diff --git a/package/Config.in b/package/Config.in
> index dac1fc568d..616fff5f69 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -2026,6 +2026,7 @@ menu "Networking"
>  	source "package/gupnp/Config.in"
>  	source "package/gupnp-av/Config.in"
>  	source "package/gupnp-dlna/Config.in"
> +	source "package/heimdal/Config.in"

The addition of heimdal as a target package should be (1) justified by
some explanation and (2) put into a separate patch.

>  	source "package/ibrcommon/Config.in"
>  	source "package/ibrdtn/Config.in"
>  	source "package/libcgi/Config.in"
> diff --git a/package/heimdal/0001-Import-AX_PROG_CC_FOR_BUILD-macro.patch b/package/heimdal/0001-Import-AX_PROG_CC_FOR_BUILD-macro.patch
> new file mode 100644
> index 0000000000..b39dc1cfd6
> --- /dev/null
> +++ b/package/heimdal/0001-Import-AX_PROG_CC_FOR_BUILD-macro.patch
> @@ -0,0 +1,176 @@
> +From 4d0d834f96cfab796fdb4653b3646feb0d306c95 Mon Sep 17 00:00:00 2001
> +From: Nicolas Williams <nico@twosigma.com>
> +Date: Sat, 24 Jun 2023 00:12:20 -0500
> +Subject: [PATCH] cf: Import AX_PROG_CC_FOR_BUILD macro
> +
> +Some versions of autoconf-archive have a broken AX_PROG_CC_FOR_BUILD.
> +
> +Signed-off-by: Nicolas Williams <nico@twosigma.com>
> +Upstream: https://github.com/heimdal/heimdal/pull/1174/commits/4d0d834f96cfab796fdb4653b3646feb0d306c95

Your Signed-off-by is missing. Why is this patch needed?


> diff --git a/package/heimdal/0002-Use-AX_PROG_CC_FOR_BUILD.patch b/package/heimdal/0002-Use-AX_PROG_CC_FOR_BUILD.patch
> new file mode 100644
> index 0000000000..27ec78587f
> --- /dev/null
> +++ b/package/heimdal/0002-Use-AX_PROG_CC_FOR_BUILD.patch
> @@ -0,0 +1,23 @@
> +From f6769797507d73d05e6a2ac2f54ff9f8f49b377c Mon Sep 17 00:00:00 2001
> +From: Nicolas Williams <nico@twosigma.com>
> +Date: Sat, 24 Jun 2023 00:15:31 -0500
> +Subject: [PATCH] cf: Use AX_PROG_CC_FOR_BUILD
> +
> +Signed-off-by: Nicolas Williams <nico@twosigma.com>
> +Upstream: https://github.com/heimdal/heimdal/pull/1174/commits/f6769797507d73d05e6a2ac2f54ff9f8f49b377c

Same comments.


> diff --git a/package/heimdal/0003-Replace-make-roken-with-roken-h-process.patch b/package/heimdal/0003-Replace-make-roken-with-roken-h-process.patch
> new file mode 100644
> index 0000000000..90acca6683
> --- /dev/null
> +++ b/package/heimdal/0003-Replace-make-roken-with-roken-h-process.patch
> @@ -0,0 +1,57 @@
> +From 28df1d8a8b5ce2a35214faad3254649a0a470bfe Mon Sep 17 00:00:00 2001
> +From: Nicolas Williams <nico@twosigma.com>
> +Date: Sat, 24 Jun 2023 22:25:00 -0500
> +Subject: [PATCH] roken: Replace make-roken with cf/roken-h-process.pl
> +
> +Signed-off-by: Nicolas Williams <nico@twosigma.com>
> +Upstream: https://github.com/heimdal/heimdal/pull/1174/commits/28df1d8a8b5ce2a35214faad3254649a0a470bfe

Ditto.


> diff --git a/package/heimdal/0004-cf-remove-comm_err-header-check.patch b/package/heimdal/0004-cf-remove-comm_err-header-check.patch
> new file mode 100644
> index 0000000000..97cef5f792
> --- /dev/null
> +++ b/package/heimdal/0004-cf-remove-comm_err-header-check.patch
> @@ -0,0 +1,48 @@
> +From acf1a8105010ecbc21a92d046028b37535c5085e Mon Sep 17 00:00:00 2001
> +From: Guillaume Chaye <guillaume.chaye@zeetim.com>
> +Date: Fri, 24 Jan 2025 16:10:14 +0100
> +Subject: [PATCH] cf: remove comm_err header check to cross-compile heimdal
> +
> +Signed-off-by: Guillaume Chaye <guillaume.chaye@zeetim.com>
> +Upstream: N/A

Upstream not available? There is a Github repository to which you can
submit PRs, so there is definitely an available upstream.


> diff --git a/package/heimdal/Config.in b/package/heimdal/Config.in
> new file mode 100644
> index 0000000000..5c71168436
> --- /dev/null
> +++ b/package/heimdal/Config.in
> @@ -0,0 +1,11 @@
> +config BR2_PACKAGE_HEIMDAL
> +	bool "my-heimdal"

Should be:

	bool "heimdal"

> +	select BR2_PACKAGE_NCURSES
> +	select BR2_PACKAGE_LIBXCRYPT

Should be:

	select BR2_PACKAGE_LIBXCRYPT if BR2_TOOLCHAIN_USES_GLIBC

> diff --git a/package/heimdal/heimdal.mk b/package/heimdal/heimdal.mk
> index f6c52f63aa..686a351c7d 100644
> --- a/package/heimdal/heimdal.mk
> +++ b/package/heimdal/heimdal.mk
> @@ -4,15 +4,20 @@
>  #
>  ################################################################################
>  
> -HEIMDAL_VERSION = f4faaeaba371fff3f8d1bc14389f5e6d70ca8e17
> +HEIMDAL_VERSION = fd2d434

Please keep a full hash, like it was already done.

>  HEIMDAL_SITE = $(call github,heimdal,heimdal,$(HEIMDAL_VERSION))
> -HOST_HEIMDAL_DEPENDENCIES = host-e2fsprogs host-ncurses host-pkgconf host-libxcrypt host-flex host-bison
> -HOST_HEIMDAL_AUTORECONF = YES
> +HEIMDAL_DEPENDENCIES=  ncurses libxcrypt host-heimdal

Space before = sign.

> +HOST_HEIMDAL_DEPENDENCIES = host-ncurses host-pkgconf host-libxcrypt host-flex host-bison

Why is host-e2fsprogs dropped?

> +HEIMDAL_AUTORECONF= YES

Space before =.

>  HEIMDAL_INSTALL_STAGING = YES

Weird that we have that in current Buildroot... as heimdal is a
host-only package.

> +HEIMDAL_CONF_ENV = MAKEINFO=true
> +HOST_HEIMDAL_CONF_ENV = $(HEIMDAL_CONF_ENV) ac_cv_prog_COMPILE_ET=no
> +
>  HOST_HEIMDAL_CONF_OPTS = \
> -	--disable-shared \
> -	--enable-static \

So the e2fsprogs issues are gone?

> +define HOST_HEIMDAL_APPLY_ADDITIONAL_PATCHES
> +	$(APPLY_PATCHES) $(@D) $(HOST_HEIMDAL_PKGDIR)/host \*.patch
>  endef

We clearly do not want to apply a different set of patches when
building the host package vs. the target package.

Could you fix all those issues, and send a new iteration, WITH a
non-empty commit log.

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

      reply	other threads:[~2025-04-21 21:49 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-02-14 12:45 [Buildroot] [PATCHv3 1/1] package/heimdal: Upgrade existing package and add target package Guillaume Chaye
2025-04-21 21:49 ` Thomas Petazzoni via buildroot [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250421234900.59e02be8@windsurf \
    --to=buildroot@buildroot.org \
    --cc=guillaume.chaye@zeetim.com \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox