All of 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.