Re: [Buildroot] [PATCH v2 1/1] package/shadow: new package

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Raphael, All,

On 2022-09-11 14:55 +0200, Raphael Pavlidis spake thusly:
> On 11.09.22 14:14, Yann E. MORIN wrote:
[--SNIP--]
> >Of course, the above is just for demonstration and mostly made up, the
> >actual commit content should be adapted. But you get the idea.
> I will try it, thanks. Technically, I need this package to use podman for
> non-root user (newuidmap and newgidmap).

You can also indeed add a (terse) explanation why that package is
useful, indeed, as that explains the purpose and can then lead to a
better understanding of the integration. The sentence above is good, but
should be rephrased to a more neutral form:

    shadow is used by podman to enable support for non-root users (with
    newuidmap and newgidmap).

(again, adapt as appropriate.)

[--SNIP--]
> >So, for shadow, I think at least the 'su' option should also depend on
> >BR2_PACKAGE_BUSYBOX_SHOW_OTHERS, if not the whole package (yet, I'd
> >vote for the whole package for simplicity sake).
> I think I understand it now. It is an option to show option or package,
> which install a non-busybox version of a binary, correct? I will add it to
> the whole package then.

Yes, you got it. :-)

[--SNIP--]
> >Also, if you did not have an actual use-case for an option, then do not
> >expose it at all. When/if someone actually has a need for that option,
> >then they can send a patch to add it.
> But this approach, I think have the disadvantage, that if it happens that
> somebody needs an option then he/she have to wait until is there, which it
> can take sometime. (Happen at least to me)
> I understand it for such options, which are useless for buildroot like if it
> is something Windows specific. IMHO, I do not see any harm to expose those
> options.

But exposing options you did not have a need for, and thus did not
exercise, means they can easily be mis-handled. Case in point: the max
length for group names.

[--SNIP--]
> >We expose options in the menuconfig only when it actually makes sense.
> >What is the purpose of limiting the group name length? Why do we want to
> >allow the user to be able to set that value, rather than let the package
> >decide?
> At least in my case, I need only BR2_PACKAGE_SHADOW_SUBORDINATE_IDS, so it
> would be nice that everything else could be deactivated to keep it small.

If you have a need for the option and you did exercise it, then that's
fine exposing it in the menuconfig.

If you want to "keep it small", then just make it so by disabling
everything, and leave it to people that actually need an option and can
test it, to add support for it.

Yes, it can take some time before a new feature lands in Buildroot.

> I tried to figure out, why this option was set to 32, and it seems that
> Linux only support username up to 32 characters. So, I will remove this
> option and set the value to 32 in the package because buildroot is only
> supporting Linux, as far as I know, correct?
> 
> https://github.com/shadow-maint/shadow/commit/1882c66bda31e50367d41b36fea41cd04fa19c73

Aha! ;-)

I won't say I knew it, but I really suspected something along those
lines. Indeed, we do not need to expose that option. It is a very good
example that exposing an untested option is not correct.

Just do not set that at all; just let the configure script use its
default. It is better to leave that untouched.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot