Buildroot Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
To: buildroot@busybox.net
Subject: [Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy
Date: Mon, 25 Feb 2019 11:20:35 +0100	[thread overview]
Message-ID: <20190225112035.4ed3ad33@windsurf.home> (raw)
In-Reply-To: <2748BB04F99E7E45A3203C831E8B20FE34D361CD@SERBE4I2.intra.erbe-med.de>

Hello Markus,

On Mon, 25 Feb 2019 10:00:37 +0000
"Steinhilber, Markus" <Markus.Steinhilber@erbe-med.com> wrote:

> > Good idea! It is simple (just one string option), and it preserves
> > backward compatibility (the default value of the option would be
> > empty).  
> 
> I have to say that I don't see a benefit in the proposed behavior
> over my patch. My patch adds also a single string option (ok, it can
> be a string list to be fair) and also doesn't break backward
> compatibility. Additionally it can be used as a rename option. With a
> default value of empty it behaves exactly as without the patch.
> 
> I also don't see it as too complex. It practically the original
> if-structure with an added if for each case to check if the value is
> set and use it or do the 'backward compatibility' way.

It's purely a matter of "design principle", i.e how Buildroot normally
does/implements/designs things. We try to be consistent and have the
same basic design principles applied across Buildroot, so that the way
package A does things is similar to how package B does things.

And nowhere in Buildroot we have this idea of two string options, where
the Nth entry of the first list "maps" to the Nth entry of the second
list. While it works, it's a bit of an awkward/strange semantic for
Config.in string options, hence our preference for a single option that
allows to specify a prefix or path where the Barebox binaries should be
installed.

So, there is nothing wrong per-se with your initial approach: it
functionally works. It is just that we (Arnout, me, as long-time
Buildroot developers/contributors) find that it doesn't fit very well
in the usual BR design principles, and there is a simple alternate
solution that works *and* fits better with the usual BR design
principles. It's as simple as that :-)

Hopefully this clarifies our suggestion on your contribution.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

  reply	other threads:[~2019-02-25 10:20 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-19 11:03 [Buildroot] [PATCH 1/1] boot/barebox: add renaming functionality to barebox image copy Steinhilber, Markus
2019-02-22  8:40 ` Thomas Petazzoni
2019-02-22  8:53   ` Arnout Vandecappelle
2019-02-22 10:19     ` Thomas Petazzoni
2019-02-25 10:00       ` Steinhilber, Markus
2019-02-25 10:20         ` Thomas Petazzoni [this message]
  -- strict thread matches above, loose matches on Subject: below --
2019-02-18 13:59 Steinhilber, Markus
2019-02-18 22:10 ` Thomas Petazzoni

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=20190225112035.4ed3ad33@windsurf.home \
    --to=thomas.petazzoni@bootlin.com \
    --cc=buildroot@busybox.net \
    /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