[Buildroot] [PATCH v2 1/1] package/earlyoom: new package

Joseph Kogut joseph.kogut at gmail.com
Wed Jun 10 23:25:40 UTC 2020


On Wed, Jun 10, 2020 at 2:31 PM Joseph Kogut <joseph.kogut at gmail.com> wrote:
>
> Hi Thomas,
>
> Great feedback, thank you for taking the time.
>
> On Wed, Jun 10, 2020 at 2:20 PM Thomas Petazzoni
> <thomas.petazzoni at bootlin.com> wrote:
> >
> > Hello Joseph,
> >
> > On Mon,  8 Jun 2020 10:58:06 -0700
> > Joseph Kogut <joseph.kogut at gmail.com> wrote:
> >
> > > diff --git a/package/earlyoom/Config.in b/package/earlyoom/Config.in
> > > new file mode 100644
> > > index 0000000000..1df1cd8f63
> > > --- /dev/null
> > > +++ b/package/earlyoom/Config.in
> > > @@ -0,0 +1,11 @@
> > > +config BR2_PACKAGE_EARLYOOM
> > > +     bool "earlyoom"
> >
> > Are you sure there are not any dependency ? I'm pretty sure at least
> > "depends on BR2_USE_MMU" is needed, as the code uses fork(). Could you
> > do a test build with ./utils/test-pkg and see what results it gives ?
> >

Looks like you were correct with the BR2_USE_MMU dependency. It also
requires C99 for some reason, which broke the build with the old (4.8)
codesourcery arm toolchain. I was able to remove this dependency with
a trivial patch, and I've submitted it upstream.

>
> I was either unaware of the existence of this tool, or had forgotten
> about it. After checking the manual, it's clearly documented, so I'll
> have to make sure to refer to that more.
>
> >
> > > +EARLYOOM_VERSION = 1.6
> > > +EARLYOOM_SITE = $(call github,rfjakob,earlyoom,v$(EARLYOOM_VERSION))
> > > +EARLYOOM_LICENSE = MIT
> > > +EARLYOOM_LICENSE_FILES = LICENSE
> > > +
> > > +EARLYOOM_MAKE_OPTS = \
> > > +     CC=$(TARGET_CC) \
> >
> > Could you use:
> >
> >         $(TARGET_CONFIGURE_OPTS)
> >
> > instead ?
> >
> > > +     DESTDIR=$(TARGET_DIR) \
> >
> > Ideally, this should be passed at install time only.
> >
> > > +     VERSION=$(EARLYOOM_VERSION) \
> > > +     PREFIX=/usr \
> > > +     SYSTEMDUNITDIR=/usr/lib/systemd/system
> > > +
> > > +define EARLYOOM_CONFIGURE_CMDS
> > > +     $(SED) "/systemctl/d" $(EARLYOOM_DIR)/Makefile
> > > +     $(SED) "/chcon/d" $(EARLYOOM_DIR)/Makefile
> > > +     $(SED) "/update-rc.d/d" $(EARLYOOM_DIR)/Makefile
> > > +     $(SED) "s/init.d\/earlyoom/init.d\/S01_earlyoom/" \
> > > +             $(EARLYOOM_DIR)/Makefile
> >
> > Can we patch the Makefile instead? Ideally, a solution that is
> > acceptable upstream would be nice. Maybe just some checks in the
> > Makefile that sees if "make install" is executed as root, and if it is,
> > do the systemctl, chcon, update-rc.d calls, but not otherwise ?
> >
>
> There are a few things I'd do differently with the Makefile. I'll see
> if I can get those changes submitted upstream, as well as include that
> patch in my next version of this package.
>
> > > +endef
> > > +
> > > +define EARLYOOM_BUILD_CMDS
> > > +     $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) earlyoom $(EARLYOOM_MAKE_OPTS)
> > > +endef
> > > +
> > > +define EARLYOOM_INSTALL_INIT_SYSV
> > > +     $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) install-initscript $(EARLYOOM_MAKE_OPTS)
> > > +endef
> > > +
> > > +define EARLYOOM_INSTALL_INIT_SYSTEMD
> > > +     $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) install $(EARLYOOM_MAKE_OPTS)
> > > +endef
> >
> > Not a big fan of not having an INSTALL_TARGET_CMDS that installs the
> > binary. Also, the init script provided by earlyoom clearly doesn't work
> > with Buildroot. It does things such as:
> >
> > . /lib/lsb/init-functions
> >
> > which doesn't exist in Buildroot.
> >
> > So I think I would prefer:
> >
> > define EARLYOOM_INSTALL_TARGET_CMDS
> >         ... $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) install-bin
> > endef
> >
> > define EARLYOOM_INSTALL_INIT_SYSTEMD
> >         manually install the earlyoom.service file
> > endef
> >
> > define EARLYOOM_INSTALL_INIT_SYSV
> >         manually install an init script provided in package/earlyoom
> > endef
> >

It also looks as though the Makefile wouldn't need modifications to
check for root as mentioned above, as this approach would skip the
privileged commands to enable the service.

> > Could you look at adjusting your patch to this ?
> >
>
> Definitely, I'll revise and resubmit.
>
>
> > Thanks a lot!
> >
> > Thomas
> > --
> > Thomas Petazzoni, CTO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com


More information about the buildroot mailing list