| Message ID | 1467388410-28135-2-git-send-email-luca@lucaceresoli.net |
|---|---|
| State | Accepted |
| Headers | show |
On 01-07-16 17:53, Luca Ceresoli wrote: > In 3d475ee0ba4d6eea6aca25594cfe5bb153ac804f a dependency on > host-pkgconf was added to host-cmake. It is a workaround to fix build > failures for packages that use pkgconf through a cmake module, but do > not depend on host-pkgconf as they should. Wouldn't this be a great time to just bite the bullet, remove the host-pkgconf dependency, and fixes packages as they break in the autobuilders? > > Since it is the package that needs host-pkgconf and not host-cmake > itself, move the dependency to the proper place, in pkg-cmake.mk. > > Also copy the explanation on the mentioned commit as a comment in > order to clarify why we do this. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> If it is decided to keep the current hack around: Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Regards, Arnout > Cc: Samuel Martin <s.martin49@gmail.com> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Davide Viti <zinosat@tiscali.it> > Cc: Arnout Vandecappelle <arnout@mind.be> > > --- > > This is done as a preliminary cleanup step for the following patch > that allows to skip building host-cmake in some cases. The dependency > where it is now would be missed when we skip building host-cmake, so I > moved it where it will be catched in all cases. Without the following > patch this can be considered just a cleanup without any visible > effect. > --- > package/cmake/cmake.mk | 2 +- > package/pkg-cmake.mk | 4 ++++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/package/cmake/cmake.mk b/package/cmake/cmake.mk > index a776b14..33fb4aa 100644 > --- a/package/cmake/cmake.mk > +++ b/package/cmake/cmake.mk > @@ -10,7 +10,7 @@ CMAKE_SITE = https://cmake.org/files/v$(CMAKE_VERSION_MAJOR) > CMAKE_LICENSE = BSD-3c > CMAKE_LICENSE_FILES = Copyright.txt > > -HOST_CMAKE_DEPENDENCIES = host-pkgconf > +HOST_CMAKE_DEPENDENCIES = > CMAKE_DEPENDENCIES = zlib jsoncpp libcurl libarchive expat bzip2 xz > > CMAKE_CONF_OPTS = \ > diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk > index 81dcfcc..8d7d265 100644 > --- a/package/pkg-cmake.mk > +++ b/package/pkg-cmake.mk > @@ -149,6 +149,10 @@ $(2)_DEPENDENCIES ?= $$(filter-out host-skeleton host-toolchain $(1),\ > $$(patsubst host-host-%,host-%,$$(addprefix host-,$$($(3)_DEPENDENCIES)))) > endif > > +# Since some CMake modules (even upstream ones) use pgk_check_modules > +# primitives to find {C,LD}FLAGS, add it to the dependency list. > +$(2)_DEPENDENCIES += host-pkgconf > + > $(2)_DEPENDENCIES += host-cmake > > # >
Arnout, All, On 2016-07-02 15:56 +0200, Arnout Vandecappelle spake thusly: > On 01-07-16 17:53, Luca Ceresoli wrote: > > In 3d475ee0ba4d6eea6aca25594cfe5bb153ac804f a dependency on > > host-pkgconf was added to host-cmake. It is a workaround to fix build > > failures for packages that use pkgconf through a cmake module, but do > > not depend on host-pkgconf as they should. > > Wouldn't this be a great time to just bite the bullet, remove the host-pkgconf > dependency, and fixes packages as they break in the autobuilders? We've jsut discussed it live here in TLS, and we've concluded that this is a topic unrelated to the work by Luca. So even if we wanted to remove this "hack", this should not block this patch from going in. However, because the culprit for causing the dependency to host-pkgconf is cmake itslef, and not a cmake-based package, I would suggest that we do not consider this a hack, but the reality. Anyway, for this patch: Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Regards, Yann E. MORIN. > > Since it is the package that needs host-pkgconf and not host-cmake > > itself, move the dependency to the proper place, in pkg-cmake.mk. > > > > Also copy the explanation on the mentioned commit as a comment in > > order to clarify why we do this. > > > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > If it is decided to keep the current hack around: > > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > > Regards, > Arnout > > > Cc: Samuel Martin <s.martin49@gmail.com> > > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > Cc: Davide Viti <zinosat@tiscali.it> > > Cc: Arnout Vandecappelle <arnout@mind.be> > > > > --- > > > > This is done as a preliminary cleanup step for the following patch > > that allows to skip building host-cmake in some cases. The dependency > > where it is now would be missed when we skip building host-cmake, so I > > moved it where it will be catched in all cases. Without the following > > patch this can be considered just a cleanup without any visible > > effect. > > --- > > package/cmake/cmake.mk | 2 +- > > package/pkg-cmake.mk | 4 ++++ > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/package/cmake/cmake.mk b/package/cmake/cmake.mk > > index a776b14..33fb4aa 100644 > > --- a/package/cmake/cmake.mk > > +++ b/package/cmake/cmake.mk > > @@ -10,7 +10,7 @@ CMAKE_SITE = https://cmake.org/files/v$(CMAKE_VERSION_MAJOR) > > CMAKE_LICENSE = BSD-3c > > CMAKE_LICENSE_FILES = Copyright.txt > > > > -HOST_CMAKE_DEPENDENCIES = host-pkgconf > > +HOST_CMAKE_DEPENDENCIES = > > CMAKE_DEPENDENCIES = zlib jsoncpp libcurl libarchive expat bzip2 xz > > > > CMAKE_CONF_OPTS = \ > > diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk > > index 81dcfcc..8d7d265 100644 > > --- a/package/pkg-cmake.mk > > +++ b/package/pkg-cmake.mk > > @@ -149,6 +149,10 @@ $(2)_DEPENDENCIES ?= $$(filter-out host-skeleton host-toolchain $(1),\ > > $$(patsubst host-host-%,host-%,$$(addprefix host-,$$($(3)_DEPENDENCIES)))) > > endif > > > > +# Since some CMake modules (even upstream ones) use pgk_check_modules > > +# primitives to find {C,LD}FLAGS, add it to the dependency list. > > +$(2)_DEPENDENCIES += host-pkgconf > > + > > $(2)_DEPENDENCIES += host-cmake > > > > # > > > > > -- > Arnout Vandecappelle arnout at mind be > Senior Embedded Software Architect +32-16-286500 > Essensium/Mind http://www.mind.be > G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven > LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle > GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
On 02-07-16 16:44, Yann E. MORIN wrote: > However, because the culprit for causing the dependency to host-pkgconf > is cmake itslef, and not a cmake-based package, I would suggest that we > do not consider this a hack, but the reality. Isn't the problem that package foo uses FindBar from cmake, which uses pkg-config to find the package? For me, the burden of depending on host-pkgconf is on package foo in that case. That said, this hack really simplifies things so let's indeed keep it. Actually, why don't we just always build host-pkgconf? On my machine 'make host-pkgconf' takes 6.5 seconds, of which 4.7 seconds are parsing the makefiles. Can we afford the cost of 1.5 seconds of build time and 844K of build size? Regards, Arnout
Arnout, All, On 2016-07-02 16:52 +0200, Arnout Vandecappelle spake thusly: > On 02-07-16 16:44, Yann E. MORIN wrote: > > However, because the culprit for causing the dependency to host-pkgconf > > is cmake itslef, and not a cmake-based package, I would suggest that we > > do not consider this a hack, but the reality. > > Isn't the problem that package foo uses FindBar from cmake, which uses > pkg-config to find the package? For me, the burden of depending on host-pkgconf > is on package foo in that case. I don't agree. ;-) The use of pkg-config is an internal detail of the CMake modules, of which the package should have no knowledge. > That said, this hack really simplifies things so let's indeed keep it. On which I'll close the discussion. Thanks! :-) > Actually, why don't we just always build host-pkgconf? On my machine 'make > host-pkgconf' takes 6.5 seconds, of which 4.7 seconds are parsing the makefiles. > Can we afford the cost of 1.5 seconds of build time and 844K of build size? If we decide so, then we should move this non-hack into the generic-package infra, so that it benefits all packages at once, not only cmake-based packages. Regards, Yann E. MORIN.
Yann, Arnout, On 02/07/2016 17:43, Yann E. MORIN wrote: > Arnout, All, > > On 2016-07-02 16:52 +0200, Arnout Vandecappelle spake thusly: >> On 02-07-16 16:44, Yann E. MORIN wrote: >>> However, because the culprit for causing the dependency to host-pkgconf >>> is cmake itslef, and not a cmake-based package, I would suggest that we >>> do not consider this a hack, but the reality. >> >> Isn't the problem that package foo uses FindBar from cmake, which uses >> pkg-config to find the package? For me, the burden of depending on host-pkgconf >> is on package foo in that case. > > I don't agree. ;-) The use of pkg-config is an internal detail of the > CMake modules, of which the package should have no knowledge. Good point, you're right. FindBar might even be not using pkgconf in the current CMake release, and start using it in a future release. As long as it's provided among the CMake built-in modules, this should be considered a CMake implementation detail. Thus my comment to the patch should be changed, but the patch content is still needed in case we skip building host-cmake. Either in the current form... > >> That said, this hack really simplifies things so let's indeed keep it. > > On which I'll close the discussion. Thanks! :-) > >> Actually, why don't we just always build host-pkgconf? On my machine 'make >> host-pkgconf' takes 6.5 seconds, of which 4.7 seconds are parsing the makefiles. >> Can we afford the cost of 1.5 seconds of build time and 844K of build size? ...or in the variant suggested by Arnout. > If we decide so, then we should move this non-hack into the > generic-package infra, so that it benefits all packages at once, not > only cmake-based packages. This might add an unneeded host-pkgconf build in a some corner cases, thus it would be once again a "hack". But its cost is so negligible that I would definitely consider it.
diff --git a/package/cmake/cmake.mk b/package/cmake/cmake.mk index a776b14..33fb4aa 100644 --- a/package/cmake/cmake.mk +++ b/package/cmake/cmake.mk @@ -10,7 +10,7 @@ CMAKE_SITE = https://cmake.org/files/v$(CMAKE_VERSION_MAJOR) CMAKE_LICENSE = BSD-3c CMAKE_LICENSE_FILES = Copyright.txt -HOST_CMAKE_DEPENDENCIES = host-pkgconf +HOST_CMAKE_DEPENDENCIES = CMAKE_DEPENDENCIES = zlib jsoncpp libcurl libarchive expat bzip2 xz CMAKE_CONF_OPTS = \ diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk index 81dcfcc..8d7d265 100644 --- a/package/pkg-cmake.mk +++ b/package/pkg-cmake.mk @@ -149,6 +149,10 @@ $(2)_DEPENDENCIES ?= $$(filter-out host-skeleton host-toolchain $(1),\ $$(patsubst host-host-%,host-%,$$(addprefix host-,$$($(3)_DEPENDENCIES)))) endif +# Since some CMake modules (even upstream ones) use pgk_check_modules +# primitives to find {C,LD}FLAGS, add it to the dependency list. +$(2)_DEPENDENCIES += host-pkgconf + $(2)_DEPENDENCIES += host-cmake #
In 3d475ee0ba4d6eea6aca25594cfe5bb153ac804f a dependency on host-pkgconf was added to host-cmake. It is a workaround to fix build failures for packages that use pkgconf through a cmake module, but do not depend on host-pkgconf as they should. Since it is the package that needs host-pkgconf and not host-cmake itself, move the dependency to the proper place, in pkg-cmake.mk. Also copy the explanation on the mentioned commit as a comment in order to clarify why we do this. Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> Cc: Samuel Martin <s.martin49@gmail.com> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Davide Viti <zinosat@tiscali.it> Cc: Arnout Vandecappelle <arnout@mind.be> --- This is done as a preliminary cleanup step for the following patch that allows to skip building host-cmake in some cases. The dependency where it is now would be missed when we skip building host-cmake, so I moved it where it will be catched in all cases. Without the following patch this can be considered just a cleanup without any visible effect. --- package/cmake/cmake.mk | 2 +- package/pkg-cmake.mk | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)