On Fri, Apr 16, 2021 at 7:28 AM Robert Joslyn
<robert.jos...@redrectangle.org> wrote:
> > On Apr 16, 2021, at 12:17 AM, Andre McCurdy <armccu...@gmail.com> wrote:
> > On Thu, Apr 15, 2021 at 10:38 PM Robert Joslyn
> > <robert.jos...@redrectangle.org> wrote:
> >>
> >> Add options to make it easier to control which features are enabled. All
> >> of these default to enabled by upstream, so keep them enabled to
> >> maintain previous behavior.
> >>
> >> The convert option also supports reiserfs, but no recipes exist in the
> >> layer index. Limit the option to ext filesystems until someone cares
> >> enough to make reiserfs recipes.
> >>
> >> Remove acl and attr from DEPENDS, as they do not apper to be needed. Add
> >> zlib since it is required.
> >>
> >> Signed-off-by: Robert Joslyn <robert.jos...@redrectangle.org>
> >> ---
> >> .../btrfs-tools/btrfs-tools_5.11.1.bb | 26 ++++++++++++++++---
> >> 1 file changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/meta/recipes-devtools/btrfs-tools/btrfs-tools_5.11.1.bb
> >> b/meta/recipes-devtools/btrfs-tools/btrfs-tools_5.11.1.bb
> >> index 2ab476a88e..b875ea1aa1 100644
> >> --- a/meta/recipes-devtools/btrfs-tools/btrfs-tools_5.11.1.bb
> >> +++ b/meta/recipes-devtools/btrfs-tools/btrfs-tools_5.11.1.bb
> >> @@ -13,7 +13,7 @@ LIC_FILES_CHKSUM = " \
> >>
> >> file://libbtrfsutil/COPYING.LESSER;md5=3000208d539ec061b899bce1d9ce9404 \
> >> "
> >> SECTION = "base"
> >> -DEPENDS = "util-linux attr e2fsprogs lzo acl"
> >> +DEPENDS = "lzo util-linux zlib"
> >> DEPENDS_append_class-target = " udev"
> >> RDEPENDS_${PN} = "libgcc"
> >>
> >> @@ -22,17 +22,37 @@ SRC_URI =
> >> "git://git.kernel.org/pub/scm/linux/kernel/git/kdave/btrfs-progs.git \
> >>
> >> file://0001-Add-a-possibility-to-specify-where-python-modules-ar.patch \
> >> "
> >>
> >> -PACKAGECONFIG ??= "python"
> >> +PACKAGECONFIG ??= " \
> >> + ${@bb.utils.filter('DISTRO_FEATURES', 'largefile', d)} \
> >
> > The largefile distro feature is for historical reference only. Recipes
> > should always enable largefile support regardless of the distro
> > feature (which will eventually be removed).
>
> Is it worth making it a PACKAGECONFIG at all, or is it preferred to just set
> —enable-largefile in EXTRA_OECONF? The upstream configure script does enable
> this by default so it’s not strictly needed at all either, but I usually
> prefer to be more explicit with configure options (as you can probably tell
> with this patch!).
Adding —enable-largefile to EXTRA_OECONF would be fine.
In general the goal of PACKAGECONFIG is not to expose every possible
configure option, but only the ones which we want to encourage people
to experiment with or use to control high level functionality. An
option to break support for large files doesn't fit into that
category.
> >> + backtrace \
> >> + programs \
> >> + shared \
> >> + convert \
> >> + python \
> >> + crypto-builtin \
> >
> > This is not conventional formatting for a list of PACKAGECONFIG
> > options. Maybe have a look at some other recipes in oe-core to get a
> > feel for the typical style.
>
> I’m not sure what you mean. Would you prefer them on one line rather than
> split? In a specific order? There seems to be a lot of different styles in
> use, so I tried to follow the OE style guide, which suggests splitting lists
> like this, but maybe I’m misunderstanding.
My preference doesn't matter much :-) I was suggesting that you look
in oe-core to get a feel for the typical style. Note that the OE style
guide mostly predates PACKAGECONFIG and hasn't been updated to cover
it in any detail.
> >> +"
> >> +PACKAGECONFIG[largefile] = "--enable-largefile,--disable-largefile"
> >> +PACKAGECONFIG[backtrace] = "--enable-backtrace,--disable-backtrace"
> >> PACKAGECONFIG[manpages] = "--enable-documentation,
> >> --disable-documentation, asciidoc-native xmlto-native"
> >> +PACKAGECONFIG[programs] = "--enable-programs,--disable-programs"
> >> +PACKAGECONFIG[shared] = "--enable-shared,--disable-shared"
> >> +PACKAGECONFIG[static] = "--enable-static,--disable-static"
> >
> > The choice to build shared and/or static libs is not something which
> > is typically controlled by PACKAGECONFIG options.
>
> Some recipes do use PACKAGECONFIG, such as ffmpeg, but in grepping the repo
> it seems more common to use EXTRA_OECONF. Is there a reason not to use
> PACKAGECONFIG here? Just that building the static libraries is uncommon? I
> don’t mind changing it, just curious.
See the comment above about the goal of PACKAGECONFIG not being to
expose every possible option. However, there are no well defined rules
about it (and in general in OE, what rules there are are not
consistently enforced) so patches like the one to ffmpeg which
converted a lot of inappropriate configure options to PACKAGECONFIG
options do sometimes get merged.
Specifically for shared and static libs, shared libs are generally
enabled by default but if a particular component didn't do that then
the normal approach would be to add a configure option to enable them
to EXTRA_OECONF. Static libs are generally unused but harmless, so
don't justify a PACKAGECONFIG option to control them. The only real
downside of enabling static libs is the wasted build time. If that
concerns you then you can try including
conf/distro/include/no-static-libs.inc in your distro config file.
> >> +PACKAGECONFIG[convert] = "--enable-convert
> >> --with-convert=ext2,--disable-convert --without-convert,e2fsprogs"
> >> PACKAGECONFIG[python] =
> >> "--enable-python,--disable-python,python3-setuptools-native"
> >> PACKAGECONFIG[zstd] = "--enable-zstd,--disable-zstd,zstd"
> >>
> >> +# Pick only one crypto provider
> >> +PACKAGECONFIG[crypto-builtin] = "--with-crypto=builtin"
> >> +PACKAGECONFIG[crypto-libgcrypt] = "--with-crypto=libgcrypt,,libgcrypt"
> >> +PACKAGECONFIG[crypto-libsodium] = "--with-crypto=libsodium,,libsodium"
> >> +PACKAGECONFIG[crypto-libkcapi] = "--with-crypto=libkcapi,,libkcapi"
> >> +
> >> inherit autotools-brokensep pkgconfig manpages
> >> inherit ${@bb.utils.contains('PACKAGECONFIG', 'python', 'distutils3-base',
> >> '', d)}
> >>
> >> CLEANBROKEN = "1"
> >>
> >> -EXTRA_OECONF_append_libc-musl = " --disable-backtrace "
> >> +PACKAGECONFIG_remove_libc-musl = "backtrace"
> >
> > Use of _remove in core recipes is discouraged.
>
> I wasn’t sure the best way to handle this. I can put it back the way it was,
> where the configure script enables the feature by default and it’s disabled
> only for musl. I could keep the packagconfig and append only for glibc:
> PACKAGECONFIG_append_libc-glibc = “ backtrace”
> I could also disable the feature by default. I do disable this in my product
> layer, but I don’t know if that is a good default.
I don't think backtrace justifies a PACKAGECONFIG option, especially
if the default value for the option is problematic (as it seems to
be). The original approach of appending --disable-backtrace to
EXTRA_OECONF for musl builds looks fine.
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#150608):
https://lists.openembedded.org/g/openembedded-core/message/150608
Mute This Topic: https://lists.openembedded.org/mt/82135972/21656
Group Owner: openembedded-core+ow...@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
