diff mbox series

[1/1] package/re2: switch to generic-package make build

Message ID 20220119084545.3642950-1-james.hilliard1@gmail.com
State Changes Requested
Headers show
Series [1/1] package/re2: switch to generic-package make build | expand

Commit Message

James Hilliard Jan. 19, 2022, 8:45 a.m. UTC
The cmake build appears to be missing features such as pkg-config
generation support, switch to the regular makefile based build
which appears to work better.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 package/re2/re2.mk | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

Comments

Yann E. MORIN Jan. 19, 2022, 8:45 p.m. UTC | #1
James, All,

On 2022-01-19 01:45 -0700, James Hilliard spake thusly:
> The cmake build appears to be missing features such as pkg-config
> generation support, switch to the regular makefile based build
> which appears to work better.

Can't we just fix the CMakeList.txt and submit a patch upstream,
instead?

Also, see below...

> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  package/re2/re2.mk | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/package/re2/re2.mk b/package/re2/re2.mk
> index b562d5d7ef..144c82339f 100644
> --- a/package/re2/re2.mk
> +++ b/package/re2/re2.mk
> @@ -10,8 +10,30 @@ RE2_LICENSE = BSD-3-Clause
>  RE2_LICENSE_FILES = LICENSE
>  RE2_INSTALL_STAGING = YES
>  
> -RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF
> -HOST_RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON
> +define RE2_BUILD_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> +		-C $(@D)
> +endef
>  
> -$(eval $(cmake-package))
> -$(eval $(host-cmake-package))
> +define RE2_INSTALL_STAGING_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> +		DESTDIR="$(STAGING_DIR)" prefix=/usr -C $(@D) install
> +endef
> +
> +define RE2_INSTALL_TARGET_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> +		DESTDIR="$(TARGET_DIR)" prefix=/usr -C $(@D) install

The 'install' target will forcibly build and install static and shared
libs:

    https://github.com/google/re2/blob/main/Makefile#L297

    296: .PHONY: install
    297: install: static-install shared-install

So, this is probably going to not play nicely for builds where a shared
build is not possible.

AFAICS, the cmake-based buildsystem got that one correct, though, so it
would be a bit of a shame to regress on that point...

Regards,
Yann E. MORIN.

> +endef
> +
> +define HOST_RE2_BUILD_CMDS
> +	$(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
> +		-C $(@D)
> +endef
> +
> +define HOST_RE2_INSTALL_CMDS
> +	$(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
> +		-C $(@D) DESTDIR="$(HOST_DIR)" prefix=/usr install
> +endef
> +
> +$(eval $(generic-package))
> +$(eval $(host-generic-package))
> -- 
> 2.25.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
James Hilliard Jan. 20, 2022, 12:42 a.m. UTC | #2
On Wed, Jan 19, 2022 at 1:45 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> James, All,
>
> On 2022-01-19 01:45 -0700, James Hilliard spake thusly:
> > The cmake build appears to be missing features such as pkg-config
> > generation support, switch to the regular makefile based build
> > which appears to work better.
>
> Can't we just fix the CMakeList.txt and submit a patch upstream,
> instead?

It looked like this had been brought up before and there was some issue that was
making this difficult to fix with cmake.
See:
https://github.com/google/re2/issues/349

>
> Also, see below...
>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> >  package/re2/re2.mk | 30 ++++++++++++++++++++++++++----
> >  1 file changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/package/re2/re2.mk b/package/re2/re2.mk
> > index b562d5d7ef..144c82339f 100644
> > --- a/package/re2/re2.mk
> > +++ b/package/re2/re2.mk
> > @@ -10,8 +10,30 @@ RE2_LICENSE = BSD-3-Clause
> >  RE2_LICENSE_FILES = LICENSE
> >  RE2_INSTALL_STAGING = YES
> >
> > -RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF
> > -HOST_RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON
> > +define RE2_BUILD_CMDS
> > +     $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> > +             -C $(@D)
> > +endef
> >
> > -$(eval $(cmake-package))
> > -$(eval $(host-cmake-package))
> > +define RE2_INSTALL_STAGING_CMDS
> > +     $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> > +             DESTDIR="$(STAGING_DIR)" prefix=/usr -C $(@D) install
> > +endef
> > +
> > +define RE2_INSTALL_TARGET_CMDS
> > +     $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> > +             DESTDIR="$(TARGET_DIR)" prefix=/usr -C $(@D) install
>
> The 'install' target will forcibly build and install static and shared
> libs:
>
>     https://github.com/google/re2/blob/main/Makefile#L297
>
>     296: .PHONY: install
>     297: install: static-install shared-install
>
> So, this is probably going to not play nicely for builds where a shared
> build is not possible.

Sent a v2 which should fix this:
https://patchwork.ozlabs.org/project/buildroot/patch/20220120003827.395469-1-james.hilliard1@gmail.com/

>
> AFAICS, the cmake-based buildsystem got that one correct, though, so it
> would be a bit of a shame to regress on that point...
>
> Regards,
> Yann E. MORIN.
>
> > +endef
> > +
> > +define HOST_RE2_BUILD_CMDS
> > +     $(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
> > +             -C $(@D)
> > +endef
> > +
> > +define HOST_RE2_INSTALL_CMDS
> > +     $(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
> > +             -C $(@D) DESTDIR="$(HOST_DIR)" prefix=/usr install
> > +endef
> > +
> > +$(eval $(generic-package))
> > +$(eval $(host-generic-package))
> > --
> > 2.25.1
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot@buildroot.org
> > https://lists.buildroot.org/mailman/listinfo/buildroot
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  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.  |
> '------------------------------^-------^------------------^--------------------'
Peter Seiderer Jan. 20, 2022, 9:29 p.m. UTC | #3
Hello James, Yann,

On Wed, 19 Jan 2022 17:42:46 -0700, James Hilliard <james.hilliard1@gmail.com> wrote:

> On Wed, Jan 19, 2022 at 1:45 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> >
> > James, All,
> >
> > On 2022-01-19 01:45 -0700, James Hilliard spake thusly:
> > > The cmake build appears to be missing features such as pkg-config
> > > generation support, switch to the regular makefile based build
> > > which appears to work better.
> >
> > Can't we just fix the CMakeList.txt and submit a patch upstream,
> > instead?
>
> It looked like this had been brought up before and there was some issue that was
> making this difficult to fix with cmake.
> See:
> https://github.com/google/re2/issues/349
>
> >
> > Also, see below...
> >
> > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > > ---
> > >  package/re2/re2.mk | 30 ++++++++++++++++++++++++++----
> > >  1 file changed, 26 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/package/re2/re2.mk b/package/re2/re2.mk
> > > index b562d5d7ef..144c82339f 100644
> > > --- a/package/re2/re2.mk
> > > +++ b/package/re2/re2.mk
> > > @@ -10,8 +10,30 @@ RE2_LICENSE = BSD-3-Clause
> > >  RE2_LICENSE_FILES = LICENSE
> > >  RE2_INSTALL_STAGING = YES
> > >
> > > -RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF
> > > -HOST_RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON
> > > +define RE2_BUILD_CMDS
> > > +     $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> > > +             -C $(@D)
> > > +endef
> > >
> > > -$(eval $(cmake-package))
> > > -$(eval $(host-cmake-package))
> > > +define RE2_INSTALL_STAGING_CMDS
> > > +     $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> > > +             DESTDIR="$(STAGING_DIR)" prefix=/usr -C $(@D) install
> > > +endef
> > > +
> > > +define RE2_INSTALL_TARGET_CMDS
> > > +     $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> > > +             DESTDIR="$(TARGET_DIR)" prefix=/usr -C $(@D) install
> >
> > The 'install' target will forcibly build and install static and shared
> > libs:
> >
> >     https://github.com/google/re2/blob/main/Makefile#L297
> >
> >     296: .PHONY: install
> >     297: install: static-install shared-install
> >
> > So, this is probably going to not play nicely for builds where a shared
> > build is not possible.
>
> Sent a v2 which should fix this:
> https://patchwork.ozlabs.org/project/buildroot/patch/20220120003827.395469-1-james.hilliard1@gmail.com/
>
> >
> > AFAICS, the cmake-based buildsystem got that one correct, though, so it
> > would be a bit of a shame to regress on that point...
> >

Or keep the cmake build and do the following to install re2.pc (taken from #349, [1]):

--- a/package/re2/re2.mk
+++ b/package/re2/re2.mk
@@ -9,9 +9,20 @@ RE2_SITE = $(call github,google,re2,$(RE2_VERSION))
 RE2_LICENSE = BSD-3-Clause
 RE2_LICENSE_FILES = LICENSE
 RE2_INSTALL_STAGING = YES
+# keep original Makefile (for re2.pc install)
+RE2_SUPPORTS_IN_SOURCE_BUILD = NO

 RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF
 HOST_RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON

+# install re2.pc
+define RE2_INSTALL_PKG_CONFIG
+	$(MAKE1) -C $(@D) DESTDIR=$(STAGING_DIR) \
+		libdir=/usr/lib includedir=/usr/include \
+		common-install
+endef
+
+RE2_POST_INSTALL_STAGING_HOOKS += RE2_INSTALL_PKG_CONFIG
+
 $(eval $(cmake-package))
 $(eval $(host-cmake-package))

Regards,
Peter

[1] https://github.com/macports/macports-ports/pull/9836#issuecomment-807868854


> > Regards,
> > Yann E. MORIN.
> >
> > > +endef
> > > +
> > > +define HOST_RE2_BUILD_CMDS
> > > +     $(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
> > > +             -C $(@D)
> > > +endef
> > > +
> > > +define HOST_RE2_INSTALL_CMDS
> > > +     $(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
> > > +             -C $(@D) DESTDIR="$(HOST_DIR)" prefix=/usr install
> > > +endef
> > > +
> > > +$(eval $(generic-package))
> > > +$(eval $(host-generic-package))
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > buildroot mailing list
> > > buildroot@buildroot.org
> > > https://lists.buildroot.org/mailman/listinfo/buildroot
> >
> > --
> > .-----------------.--------------------.------------------.--------------------.
> > |  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
James Hilliard Jan. 20, 2022, 11:57 p.m. UTC | #4
On Thu, Jan 20, 2022 at 2:29 PM Peter Seiderer <ps.report@gmx.net> wrote:
>
> Hello James, Yann,
>
> On Wed, 19 Jan 2022 17:42:46 -0700, James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > On Wed, Jan 19, 2022 at 1:45 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > >
> > > James, All,
> > >
> > > On 2022-01-19 01:45 -0700, James Hilliard spake thusly:
> > > > The cmake build appears to be missing features such as pkg-config
> > > > generation support, switch to the regular makefile based build
> > > > which appears to work better.
> > >
> > > Can't we just fix the CMakeList.txt and submit a patch upstream,
> > > instead?
> >
> > It looked like this had been brought up before and there was some issue that was
> > making this difficult to fix with cmake.
> > See:
> > https://github.com/google/re2/issues/349
> >
> > >
> > > Also, see below...
> > >
> > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > > > ---
> > > >  package/re2/re2.mk | 30 ++++++++++++++++++++++++++----
> > > >  1 file changed, 26 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/package/re2/re2.mk b/package/re2/re2.mk
> > > > index b562d5d7ef..144c82339f 100644
> > > > --- a/package/re2/re2.mk
> > > > +++ b/package/re2/re2.mk
> > > > @@ -10,8 +10,30 @@ RE2_LICENSE = BSD-3-Clause
> > > >  RE2_LICENSE_FILES = LICENSE
> > > >  RE2_INSTALL_STAGING = YES
> > > >
> > > > -RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF
> > > > -HOST_RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON
> > > > +define RE2_BUILD_CMDS
> > > > +     $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> > > > +             -C $(@D)
> > > > +endef
> > > >
> > > > -$(eval $(cmake-package))
> > > > -$(eval $(host-cmake-package))
> > > > +define RE2_INSTALL_STAGING_CMDS
> > > > +     $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> > > > +             DESTDIR="$(STAGING_DIR)" prefix=/usr -C $(@D) install
> > > > +endef
> > > > +
> > > > +define RE2_INSTALL_TARGET_CMDS
> > > > +     $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> > > > +             DESTDIR="$(TARGET_DIR)" prefix=/usr -C $(@D) install
> > >
> > > The 'install' target will forcibly build and install static and shared
> > > libs:
> > >
> > >     https://github.com/google/re2/blob/main/Makefile#L297
> > >
> > >     296: .PHONY: install
> > >     297: install: static-install shared-install
> > >
> > > So, this is probably going to not play nicely for builds where a shared
> > > build is not possible.
> >
> > Sent a v2 which should fix this:
> > https://patchwork.ozlabs.org/project/buildroot/patch/20220120003827.395469-1-james.hilliard1@gmail.com/
> >
> > >
> > > AFAICS, the cmake-based buildsystem got that one correct, though, so it
> > > would be a bit of a shame to regress on that point...
> > >
>
> Or keep the cmake build and do the following to install re2.pc (taken from #349, [1]):
>
> --- a/package/re2/re2.mk
> +++ b/package/re2/re2.mk
> @@ -9,9 +9,20 @@ RE2_SITE = $(call github,google,re2,$(RE2_VERSION))
>  RE2_LICENSE = BSD-3-Clause
>  RE2_LICENSE_FILES = LICENSE
>  RE2_INSTALL_STAGING = YES
> +# keep original Makefile (for re2.pc install)
> +RE2_SUPPORTS_IN_SOURCE_BUILD = NO
>
>  RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF
>  HOST_RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON
>
> +# install re2.pc
> +define RE2_INSTALL_PKG_CONFIG
> +       $(MAKE1) -C $(@D) DESTDIR=$(STAGING_DIR) \
> +               libdir=/usr/lib includedir=/usr/include \
> +               common-install
> +endef
> +
> +RE2_POST_INSTALL_STAGING_HOOKS += RE2_INSTALL_PKG_CONFIG
> +
>  $(eval $(cmake-package))
>  $(eval $(host-cmake-package))
>
> Regards,
> Peter
>
> [1] https://github.com/macports/macports-ports/pull/9836#issuecomment-807868854

There's a problem with this approach, the makefile with common-install
gets overwritten
by the one generated by cmake if cmake is used.

>
>
> > > Regards,
> > > Yann E. MORIN.
> > >
> > > > +endef
> > > > +
> > > > +define HOST_RE2_BUILD_CMDS
> > > > +     $(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
> > > > +             -C $(@D)
> > > > +endef
> > > > +
> > > > +define HOST_RE2_INSTALL_CMDS
> > > > +     $(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
> > > > +             -C $(@D) DESTDIR="$(HOST_DIR)" prefix=/usr install
> > > > +endef
> > > > +
> > > > +$(eval $(generic-package))
> > > > +$(eval $(host-generic-package))
> > > > --
> > > > 2.25.1
> > > >
> > > > _______________________________________________
> > > > buildroot mailing list
> > > > buildroot@buildroot.org
> > > > https://lists.buildroot.org/mailman/listinfo/buildroot
> > >
> > > --
> > > .-----------------.--------------------.------------------.--------------------.
> > > |  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
>
Yann E. MORIN Jan. 21, 2022, 6:58 a.m. UTC | #5
James, All,

On 2022-01-20 16:57 -0700, James Hilliard spake thusly:
> On Thu, Jan 20, 2022 at 2:29 PM Peter Seiderer <ps.report@gmx.net> wrote:
> > Or keep the cmake build and do the following to install re2.pc (taken from #349, [1]):
> >
> > --- a/package/re2/re2.mk
> > +++ b/package/re2/re2.mk
> > @@ -9,9 +9,20 @@ RE2_SITE = $(call github,google,re2,$(RE2_VERSION))
> >  RE2_LICENSE = BSD-3-Clause
> >  RE2_LICENSE_FILES = LICENSE
> >  RE2_INSTALL_STAGING = YES
> > +# keep original Makefile (for re2.pc install)
> > +RE2_SUPPORTS_IN_SOURCE_BUILD = NO
> >
> >  RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF
> >  HOST_RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON
> >
> > +# install re2.pc
> > +define RE2_INSTALL_PKG_CONFIG
> > +       $(MAKE1) -C $(@D) DESTDIR=$(STAGING_DIR) \
> > +               libdir=/usr/lib includedir=/usr/include \
> > +               common-install
> > +endef
> > +
> > +RE2_POST_INSTALL_STAGING_HOOKS += RE2_INSTALL_PKG_CONFIG
> > +
> >  $(eval $(cmake-package))
> >  $(eval $(host-cmake-package))
> >
> > Regards,
> > Peter
> >
> > [1] https://github.com/macports/macports-ports/pull/9836#issuecomment-807868854
> 
> There's a problem with this approach, the makefile with common-install
> gets overwritten
> by the one generated by cmake if cmake is used.

It currently gets overwritten, because the build is done in-tree, but
What Peter suggests is doing the build out-of-tree, with:
    RE2_SUPPORTS_IN_SOURCE_BUILD = NO

However, I can't say that I am very fond of this option either. Misinx
the two buildsystems looks icky... What bothers me a bit, is that
common-install also installs the headers, not just the re2.pc, so this
could, even if unlikely, conflict with what the cmake-side would do.

But when looking at the various bugs about this issue:

    https://github.com/google/re2/issues/349
     -> https://github.com/google/re2/issues/304#issuecomment-808219200
         -> https://github.com/macports/macports-ports/pull/9836

    re2: change to cmake build to support gprc, which currently needs a
    cmake version of re2 to build. Try to make it work like the previous
    autotools build worked.

So, that last issue, against homebrew, is interesting: it seems that
using the Makefile-based buildsystem is not enough, because it does not
isntall the cmake equivalent of re2.pc, which breaks grpc.

So, whether we keep the cmake build, or switch to the Makefile build, we
would eventually still miss some pieces...

Damn, back to square-one...

So, I think that Peter's proposal is not so un-appealing, in then end.
But then in such situation, I think it would be simpler for us to just
apply something like:

    define RE2_INSTALL_PC
        $(INTALL) -d -m 0644 $(@D)/re2.pc \
            $(STAGING_DIR)/usr/lib/pkgconfig/re2.pc
        $(SED) 's,@includedir@,/usr/include,; s,@libdir@,/usr/lib,' \
            $(STAGING_DIR)/usr/lib/pkgconfig/re2.pc
    endef
    RE2_POST_STAGING_INSTALL_HOOKS += RE2_INSTALL_PC

And be done with it: minimal change to the re2 package, no dual-buildsystem
situation and thus no install conflict on the headers, usual and simple
post-install hook.

Thoughts?

Regards,
Yann E. MORIN.
Thomas Petazzoni Jan. 21, 2022, 7:41 a.m. UTC | #6
On Fri, 21 Jan 2022 07:58:50 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> So, I think that Peter's proposal is not so un-appealing, in then end.
> But then in such situation, I think it would be simpler for us to just
> apply something like:
> 
>     define RE2_INSTALL_PC
>         $(INTALL) -d -m 0644 $(@D)/re2.pc \
>             $(STAGING_DIR)/usr/lib/pkgconfig/re2.pc
>         $(SED) 's,@includedir@,/usr/include,; s,@libdir@,/usr/lib,' \
>             $(STAGING_DIR)/usr/lib/pkgconfig/re2.pc
>     endef
>     RE2_POST_STAGING_INSTALL_HOOKS += RE2_INSTALL_PC
> 
> And be done with it: minimal change to the re2 package, no dual-buildsystem
> situation and thus no install conflict on the headers, usual and simple
> post-install hook.

I might be missing something in the whole discussion, but why not patch
the CMake build system so that it also installs this .pc file, and
contribute this fix upstream ?

Best regards,

Thomas
Yann E. MORIN Jan. 21, 2022, 7:59 a.m. UTC | #7
Thomas, All,

On 2022-01-21 08:41 +0100, Thomas Petazzoni spake thusly:
> On Fri, 21 Jan 2022 07:58:50 +0100
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> 
> > So, I think that Peter's proposal is not so un-appealing, in then end.
> > But then in such situation, I think it would be simpler for us to just
> > apply something like:
> > 
> >     define RE2_INSTALL_PC
> >         $(INTALL) -d -m 0644 $(@D)/re2.pc \
> >             $(STAGING_DIR)/usr/lib/pkgconfig/re2.pc
> >         $(SED) 's,@includedir@,/usr/include,; s,@libdir@,/usr/lib,' \
> >             $(STAGING_DIR)/usr/lib/pkgconfig/re2.pc
> >     endef
> >     RE2_POST_STAGING_INSTALL_HOOKS += RE2_INSTALL_PC
> > 
> > And be done with it: minimal change to the re2 package, no dual-buildsystem
> > situation and thus no install conflict on the headers, usual and simple
> > post-install hook.
> 
> I might be missing something in the whole discussion, but why not patch
> the CMake build system so that it also installs this .pc file, and
> contribute this fix upstream ?

Because upstream have not been very enthusisatic about it, as James
pointed out when I suggested the same. ;-)

Regards,
Yann E. MORIN.
James Hilliard Jan. 21, 2022, 9:17 a.m. UTC | #8
On Thu, Jan 20, 2022 at 11:58 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> James, All,
>
> On 2022-01-20 16:57 -0700, James Hilliard spake thusly:
> > On Thu, Jan 20, 2022 at 2:29 PM Peter Seiderer <ps.report@gmx.net> wrote:
> > > Or keep the cmake build and do the following to install re2.pc (taken from #349, [1]):
> > >
> > > --- a/package/re2/re2.mk
> > > +++ b/package/re2/re2.mk
> > > @@ -9,9 +9,20 @@ RE2_SITE = $(call github,google,re2,$(RE2_VERSION))
> > >  RE2_LICENSE = BSD-3-Clause
> > >  RE2_LICENSE_FILES = LICENSE
> > >  RE2_INSTALL_STAGING = YES
> > > +# keep original Makefile (for re2.pc install)
> > > +RE2_SUPPORTS_IN_SOURCE_BUILD = NO
> > >
> > >  RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF
> > >  HOST_RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON
> > >
> > > +# install re2.pc
> > > +define RE2_INSTALL_PKG_CONFIG
> > > +       $(MAKE1) -C $(@D) DESTDIR=$(STAGING_DIR) \
> > > +               libdir=/usr/lib includedir=/usr/include \
> > > +               common-install
> > > +endef
> > > +
> > > +RE2_POST_INSTALL_STAGING_HOOKS += RE2_INSTALL_PKG_CONFIG
> > > +
> > >  $(eval $(cmake-package))
> > >  $(eval $(host-cmake-package))
> > >
> > > Regards,
> > > Peter
> > >
> > > [1] https://github.com/macports/macports-ports/pull/9836#issuecomment-807868854
> >
> > There's a problem with this approach, the makefile with common-install
> > gets overwritten
> > by the one generated by cmake if cmake is used.
>
> It currently gets overwritten, because the build is done in-tree, but
> What Peter suggests is doing the build out-of-tree, with:
>     RE2_SUPPORTS_IN_SOURCE_BUILD = NO
>
> However, I can't say that I am very fond of this option either. Misinx
> the two buildsystems looks icky... What bothers me a bit, is that
> common-install also installs the headers, not just the re2.pc, so this
> could, even if unlikely, conflict with what the cmake-side would do.
>
> But when looking at the various bugs about this issue:
>
>     https://github.com/google/re2/issues/349
>      -> https://github.com/google/re2/issues/304#issuecomment-808219200
>          -> https://github.com/macports/macports-ports/pull/9836
>
>     re2: change to cmake build to support gprc, which currently needs a
>     cmake version of re2 to build. Try to make it work like the previous
>     autotools build worked.
>
> So, that last issue, against homebrew, is interesting: it seems that
> using the Makefile-based buildsystem is not enough, because it does not
> isntall the cmake equivalent of re2.pc, which breaks grpc.

That appears to have been fixed:
https://github.com/grpc/grpc/commit/45e413d2520795e7281e9a592af81620349bc186

>
> So, whether we keep the cmake build, or switch to the Makefile build, we
> would eventually still miss some pieces...
>
> Damn, back to square-one...
>
> So, I think that Peter's proposal is not so un-appealing, in then end.
> But then in such situation, I think it would be simpler for us to just
> apply something like:
>
>     define RE2_INSTALL_PC
>         $(INTALL) -d -m 0644 $(@D)/re2.pc \
>             $(STAGING_DIR)/usr/lib/pkgconfig/re2.pc
>         $(SED) 's,@includedir@,/usr/include,; s,@libdir@,/usr/lib,' \
>             $(STAGING_DIR)/usr/lib/pkgconfig/re2.pc
>     endef
>     RE2_POST_STAGING_INSTALL_HOOKS += RE2_INSTALL_PC
>
> And be done with it: minimal change to the re2 package, no dual-buildsystem
> situation and thus no install conflict on the headers, usual and simple
> post-install hook.
>
> Thoughts?
>
> 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.  |
> '------------------------------^-------^------------------^--------------------'
Thomas Petazzoni Jan. 21, 2022, 1:26 p.m. UTC | #9
On Fri, 21 Jan 2022 08:59:24 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> > I might be missing something in the whole discussion, but why not patch
> > the CMake build system so that it also installs this .pc file, and
> > contribute this fix upstream ?  
> 
> Because upstream have not been very enthusisatic about it, as James
> pointed out when I suggested the same. ;-)

Upstream is not enthusiastic about supporting pkg-config, about having
their two possible build systems behave in the same way? What sort of
odd upstream is that?

Thomas
James Hilliard Jan. 21, 2022, 9:16 p.m. UTC | #10
On Fri, Jan 21, 2022 at 6:26 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Fri, 21 Jan 2022 08:59:24 +0100
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
>
> > > I might be missing something in the whole discussion, but why not patch
> > > the CMake build system so that it also installs this .pc file, and
> > > contribute this fix upstream ?
> >
> > Because upstream have not been very enthusisatic about it, as James
> > pointed out when I suggested the same. ;-)
>
> Upstream is not enthusiastic about supporting pkg-config, about having
> their two possible build systems behave in the same way? What sort of
> odd upstream is that?

Best I can tell the cmake build is mostly intended for non-system
builds/installs
or something weird like that.

>
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com
diff mbox series

Patch

diff --git a/package/re2/re2.mk b/package/re2/re2.mk
index b562d5d7ef..144c82339f 100644
--- a/package/re2/re2.mk
+++ b/package/re2/re2.mk
@@ -10,8 +10,30 @@  RE2_LICENSE = BSD-3-Clause
 RE2_LICENSE_FILES = LICENSE
 RE2_INSTALL_STAGING = YES
 
-RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF
-HOST_RE2_CONF_OPTS += -DRE2_BUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON
+define RE2_BUILD_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
+		-C $(@D)
+endef
 
-$(eval $(cmake-package))
-$(eval $(host-cmake-package))
+define RE2_INSTALL_STAGING_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
+		DESTDIR="$(STAGING_DIR)" prefix=/usr -C $(@D) install
+endef
+
+define RE2_INSTALL_TARGET_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
+		DESTDIR="$(TARGET_DIR)" prefix=/usr -C $(@D) install
+endef
+
+define HOST_RE2_BUILD_CMDS
+	$(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
+		-C $(@D)
+endef
+
+define HOST_RE2_INSTALL_CMDS
+	$(HOST_MAKE_ENV) $(MAKE) $(HOST_CONFIGURE_OPTS) \
+		-C $(@D) DESTDIR="$(HOST_DIR)" prefix=/usr install
+endef
+
+$(eval $(generic-package))
+$(eval $(host-generic-package))