| Message ID | 20220727071557.3434892-1-james.hilliard1@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v5,1/2] support/misc/toolchainfile.cmake.in: don't set PKG_CONFIG_SYSROOT_DIR | expand |
Hello James, On Wed, 27 Jul 2022 01:15:56 -0600 James Hilliard <james.hilliard1@gmail.com> wrote: > This doesn't appear to be required and seems to break for packages > using meson's pkgconfig.relocatable format. Thanks for your contribution. However, if you want us to merge this can of contribution needs much, much, much better commit logs and explanations. An explanation that starts with "this doesn't appear to be required" and continues with "seems to break" is the opposite of a convincing explanation. Here is the sort of commit log that we need: """ Defining PKG_CONFIG_SYSROOT_DIR in the CMake toolchain file is not needed because it is already passed... In addition, passing this in the CMake toolchain file causes breakage of the following defconfig, once meson's pkgconfig.relocatable is enabled, with the following failure: [...] This failure occurs because ... """ Note: I agree that passing PKG_CONFIG_SYSROOT_DIR in the CMake toolchain file is probably not needed because our pkg-config wrapper already passes the right value. However, I fail to see how that can make a difference. How something in the CMake toolchain file can affect the build of Meson based packages? We very much appreciate all the improvements you are submitting, many of them are very relevant and very useful. However, to have them merged, we need detailed and convincing argumentation, no vague commit logs that seem to indicate the change is actually papering over a problem rather than addressing it for real. Thanks! Thomas
On Sun, Aug 7, 2022 at 7:54 AM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello James, > > On Wed, 27 Jul 2022 01:15:56 -0600 > James Hilliard <james.hilliard1@gmail.com> wrote: > > > This doesn't appear to be required and seems to break for packages > > using meson's pkgconfig.relocatable format. > > Thanks for your contribution. However, if you want us to merge this can > of contribution needs much, much, much better commit logs and > explanations. > > An explanation that starts with "this doesn't appear to be required" > and continues with "seems to break" is the opposite of a convincing > explanation. > > Here is the sort of commit log that we need: > > """ > Defining PKG_CONFIG_SYSROOT_DIR in the CMake toolchain file is not > needed because it is already passed... > > In addition, passing this in the CMake toolchain file causes breakage > of the following defconfig, once meson's pkgconfig.relocatable is > enabled, with the following failure: > > [...] > > This failure occurs because ... > """ > > Note: I agree that passing PKG_CONFIG_SYSROOT_DIR in the CMake > toolchain file is probably not needed because our pkg-config wrapper > already passes the right value. However, I fail to see how that can > make a difference. How something in the CMake toolchain file can affect > the build of Meson based packages? I think it's an issue with how it interacts with meson's pkg-config prefix in relocatable mode AFAIU, seems cmake wasn't setting the prefix correctly, which generally only seemed to cause issues with PKG_CONFIG_SYSROOT_DIR from what I could tell. I recall someone mentioned PKG_CONFIG_SYSROOT_DIR was needed for external toolchains or something along those lines so I came up with an alternative approach to avoid removing PKG_CONFIG_SYSROOT_DIR. > > We very much appreciate all the improvements you are submitting, many > of them are very relevant and very useful. However, to have them > merged, we need detailed and convincing argumentation, no vague commit > logs that seem to indicate the change is actually papering over a > problem rather than addressing it for real. This was superseded by: https://patchwork.ozlabs.org/project/buildroot/patch/20220728014402.142320-1-james.hilliard1@gmail.com/ https://patchwork.ozlabs.org/project/buildroot/patch/20220728014402.142320-2-james.hilliard1@gmail.com/ I separated the pkg-config and relocatable format change out from the ninja to meson build/install migration. > > Thanks! > > Thomas > -- > Thomas Petazzoni, co-owner and CEO, Bootlin > Embedded Linux and Kernel engineering and training > https://bootlin.com
On Mon, Aug 8, 2022 at 1:15 AM James Hilliard <james.hilliard1@gmail.com> wrote: > > On Sun, Aug 7, 2022 at 7:54 AM Thomas Petazzoni > <thomas.petazzoni@bootlin.com> wrote: > > > > Hello James, > > > > On Wed, 27 Jul 2022 01:15:56 -0600 > > James Hilliard <james.hilliard1@gmail.com> wrote: > > > > > This doesn't appear to be required and seems to break for packages > > > using meson's pkgconfig.relocatable format. > > > > Thanks for your contribution. However, if you want us to merge this can > > of contribution needs much, much, much better commit logs and > > explanations. > > > > An explanation that starts with "this doesn't appear to be required" > > and continues with "seems to break" is the opposite of a convincing > > explanation. > > > > Here is the sort of commit log that we need: > > > > """ > > Defining PKG_CONFIG_SYSROOT_DIR in the CMake toolchain file is not > > needed because it is already passed... > > > > In addition, passing this in the CMake toolchain file causes breakage > > of the following defconfig, once meson's pkgconfig.relocatable is > > enabled, with the following failure: > > > > [...] > > > > This failure occurs because ... > > """ > > > > Note: I agree that passing PKG_CONFIG_SYSROOT_DIR in the CMake > > toolchain file is probably not needed because our pkg-config wrapper > > already passes the right value. However, I fail to see how that can > > make a difference. How something in the CMake toolchain file can affect > > the build of Meson based packages? > > I think it's an issue with how it interacts with meson's pkg-config prefix in > relocatable mode AFAIU, seems cmake wasn't setting the prefix correctly, > which generally only seemed to cause issues with PKG_CONFIG_SYSROOT_DIR > from what I could tell. > > I recall someone mentioned PKG_CONFIG_SYSROOT_DIR was needed for > external toolchains or something along those lines so I came up with > an alternative > approach to avoid removing PKG_CONFIG_SYSROOT_DIR. Oh, it was apparently needed by external builds using the SDK, see: https://lore.kernel.org/buildroot/23a6eba2-06fd-8579-9815-214b3656daf7@mind.be/ > > > > > We very much appreciate all the improvements you are submitting, many > > of them are very relevant and very useful. However, to have them > > merged, we need detailed and convincing argumentation, no vague commit > > logs that seem to indicate the change is actually papering over a > > problem rather than addressing it for real. > > This was superseded by: > https://patchwork.ozlabs.org/project/buildroot/patch/20220728014402.142320-1-james.hilliard1@gmail.com/ I avoided removing PKG_CONFIG_SYSROOT_DIR due to the SDK issue. > https://patchwork.ozlabs.org/project/buildroot/patch/20220728014402.142320-2-james.hilliard1@gmail.com/ > > I separated the pkg-config and relocatable format change out from the ninja > to meson build/install migration. > > > > > Thanks! > > > > Thomas > > -- > > Thomas Petazzoni, co-owner and CEO, Bootlin > > Embedded Linux and Kernel engineering and training > > https://bootlin.com
diff --git a/support/misc/toolchainfile.cmake.in b/support/misc/toolchainfile.cmake.in index 5d2b8695b4..24d2b401f0 100644 --- a/support/misc/toolchainfile.cmake.in +++ b/support/misc/toolchainfile.cmake.in @@ -91,7 +91,6 @@ endif() if(NOT DEFINED CMAKE_FIND_ROOT_PATH_MODE_INCLUDE) set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY) endif() -set(ENV{PKG_CONFIG_SYSROOT_DIR} "${RELOCATED_HOST_DIR}/@@STAGING_SUBDIR@@") # This toolchain file can be used both inside and outside Buildroot. if(NOT DEFINED CMAKE_C_COMPILER)
This doesn't appear to be required and seems to break for packages using meson's pkgconfig.relocatable format. Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- support/misc/toolchainfile.cmake.in | 1 - 1 file changed, 1 deletion(-)