| Message ID | 1457529409-24102-1-git-send-email-gustavo@zacarias.com.ar |
|---|---|
| State | Changes Requested |
| Headers | show |
>>>>> "Gustavo" == Gustavo Zacarias <gustavo@zacarias.com.ar> writes: > Use shell ${STAGING_DIR} to expand at run time instead of build time > hence avoiding hardcoding the staging directory in it and making it > relocatable. > Signed-off-by: Gustavo Zacarias <gustavo@zacarias.com.ar> Ehh, doesn't this break using pkg-config outside buildroot? > --- > package/pkgconf/pkg-config.in | 2 +- > package/pkgconf/pkgconf.mk | 3 --- > 2 files changed, 1 insertion(+), 4 deletions(-) > diff --git a/package/pkgconf/pkg-config.in b/package/pkgconf/pkg-config.in > index 4dec487..51ac8c1 100644 > --- a/package/pkgconf/pkg-config.in > +++ b/package/pkgconf/pkg-config.in > @@ -1,2 +1,2 @@ > #!/bin/sh > -PKG_CONFIG_LIBDIR=${PKG_CONFIG_LIBDIR:-@PKG_CONFIG_LIBDIR@} PKG_CONFIG_SYSROOT_DIR=${PKG_CONFIG_SYSROOT_DIR:-@STAGING_DIR@} $(dirname $0)/pkgconf @STATIC@ $@ > +PKG_CONFIG_LIBDIR=${PKG_CONFIG_LIBDIR:-${STAGING_DIR}/usr/lib/pkgconfig:${STAGING_DIR}/usr/share/pkgconfig} PKG_CONFIG_SYSROOT_DIR=${PKG_CONFIG_SYSROOT_DIR:-${STAGING_DIR}} $(dirname $0)/pkgconf @STATIC@ $@ > diff --git a/package/pkgconf/pkgconf.mk b/package/pkgconf/pkgconf.mk > index c8b0cba..3cfcc2a 100644 > --- a/package/pkgconf/pkgconf.mk > +++ b/package/pkgconf/pkgconf.mk > @@ -19,9 +19,6 @@ endef > define HOST_PKGCONF_INSTALL_WRAPPER > $(INSTALL) -m 0755 -D package/pkgconf/pkg-config.in \ > $(HOST_DIR)/usr/bin/pkg-config > - $(SED) 's,@PKG_CONFIG_LIBDIR@,$(STAGING_DIR)/usr/lib/pkgconfig:$(STAGING_DIR)/usr/share/pkgconfig,' \ > - -e 's,@STAGING_DIR@,$(STAGING_DIR),' \ > - $(HOST_DIR)/usr/bin/pkg-config > endef > define HOST_PKGCONF_STATIC > -- > 2.4.10 > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
On 09/03/16 10:46, Peter Korsgaard wrote: >>>>>> "Gustavo" == Gustavo Zacarias <gustavo@zacarias.com.ar> writes: > > > Use shell ${STAGING_DIR} to expand at run time instead of build time > > hence avoiding hardcoding the staging directory in it and making it > > relocatable. > > > Signed-off-by: Gustavo Zacarias <gustavo@zacarias.com.ar> > > Ehh, doesn't this break using pkg-config outside buildroot? Hi. Indeed it does, but you can't have the cake and eat it at once can you? Arnout in the famous vala-wrapper thread pointed that we shouldn't hardcode the staging directory in it and i've adjusted accordingly, so which way is it? (same logic would apply to both cases). Re: http://lists.busybox.net/pipermail/buildroot/2016-February/153671.html Regards.
>>>>> "Gustavo" == Gustavo Zacarias <gustavo@zacarias.com.ar> writes: Hi, >> Ehh, doesn't this break using pkg-config outside buildroot? > Hi. > Indeed it does, but you can't have the cake and eat it at once can you? Well, we cannot really break existing features to add new ones. The relative location between the pkg-config wrapper script and the staging directory is constant (or rather known at build time), so I guess we can do something with /proc/self/exe. We might need to implement the wrapper in C instead for that to work as /proc/self/exe for a shell script seems to return /bin/dash here. > Arnout in the famous vala-wrapper thread pointed that we shouldn't > hardcode the staging directory in it and i've adjusted accordingly, so > which way is it? (same logic would apply to both cases). Re: > http://lists.busybox.net/pipermail/buildroot/2016-February/153671.html > Regards. I know people today are already using the pkg-config wrapper outside buildroot (E.G. the output/host dir is used as a sdk), so we imho cannot break that. It would be nice to also fix it for vala-wrapper, but as it hasn't worked before it is atleast not a regression if it doesn't work.
Hello, On Wed, 9 Mar 2016 10:51:52 -0300, Gustavo Zacarias wrote: > Indeed it does, but you can't have the cake and eat it at once can you? > Arnout in the famous vala-wrapper thread pointed that we shouldn't > hardcode the staging directory in it and i've adjusted accordingly, so > which way is it? (same logic would apply to both cases). Re: > http://lists.busybox.net/pipermail/buildroot/2016-February/153671.html What about doing this relatively to the wrapper location? WRAPPER_DIR=$(dirname $0) STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../../../staging/) and there you are, it is both relocatable and doesn't rely on STAGING_DIR being defined in the environment. Thomas
On 09/03/16 11:00, Peter Korsgaard wrote: > Well, we cannot really break existing features to add new ones. The > relative location between the pkg-config wrapper script and the staging > directory is constant (or rather known at build time), so I guess we can > do something with /proc/self/exe. We might need to implement the wrapper > in C instead for that to work as /proc/self/exe for a shell script seems > to return /bin/dash here. That's overly complicated for no value at all. You know the sysroot is in the host directory as well, so you just need to filter out /usr/bin from $0 and hardcode the tuple in the wrapper - that's guaranteed to not change, otherwise you're in serious trouble. Regards.
On 09/03/16 11:03, Thomas Petazzoni wrote: > What about doing this relatively to the wrapper location? > > WRAPPER_DIR=$(dirname $0) > STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../../../staging/) > > and there you are, it is both relocatable and doesn't rely on > STAGING_DIR being defined in the environment. > > Thomas Hi. That won't work, since staging is an absolute symlink, so when moving the SDK around it will be broken and fail. You need something like: WRAPPER_DIR=$(dirname $0) STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../@TUPLE@/sysroot) And hardcode tuple which isn't likely to change. Regards.
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: > Hello, > On Wed, 9 Mar 2016 10:51:52 -0300, Gustavo Zacarias wrote: >> Indeed it does, but you can't have the cake and eat it at once can you? >> Arnout in the famous vala-wrapper thread pointed that we shouldn't >> hardcode the staging directory in it and i've adjusted accordingly, so >> which way is it? (same logic would apply to both cases). Re: >> http://lists.busybox.net/pipermail/buildroot/2016-February/153671.html > What about doing this relatively to the wrapper location? > WRAPPER_DIR=$(dirname $0) That relies on argv[0] containing the full path, which it normally will do when you run it from the shell, but isn't guaranteed to work. If we want to support that then we should use /proc/self/exe > STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../../../staging/) This will only work if you keep the entire buildroot output around (or rather output/staging) and don't customize the BR2_HOST_DIR.
>>>>> "Gustavo" == Gustavo Zacarias <gustavo@zacarias.com.ar> writes: > On 09/03/16 11:00, Peter Korsgaard wrote: >> Well, we cannot really break existing features to add new ones. The >> relative location between the pkg-config wrapper script and the staging >> directory is constant (or rather known at build time), so I guess we can >> do something with /proc/self/exe. We might need to implement the wrapper >> in C instead for that to work as /proc/self/exe for a shell script seems >> to return /bin/dash here. > That's overly complicated for no value at all. > You know the sysroot is in the host directory as well, so you just > need to filter out /usr/bin from $0 and hardcode the tuple in the > wrapper - > that's guaranteed to not change, otherwise you're in serious trouble. Yes, that's basically what I'm saying +/- the question if you want to rely on $0 containing the full path to the wrapper or if you use /proc/self/exe instead.
On 03/09/16 15:31, Peter Korsgaard wrote: >>>>>> "Gustavo" == Gustavo Zacarias <gustavo@zacarias.com.ar> writes: > > > On 09/03/16 11:00, Peter Korsgaard wrote: > >> Well, we cannot really break existing features to add new ones. The > >> relative location between the pkg-config wrapper script and the staging > >> directory is constant (or rather known at build time), so I guess we can > >> do something with /proc/self/exe. We might need to implement the wrapper > >> in C instead for that to work as /proc/self/exe for a shell script seems > >> to return /bin/dash here. > > > That's overly complicated for no value at all. > > You know the sysroot is in the host directory as well, so you just > > need to filter out /usr/bin from $0 and hardcode the tuple in the > > wrapper - > > that's guaranteed to not change, otherwise you're in serious trouble. > > Yes, that's basically what I'm saying +/- the question if you want to > rely on $0 containing the full path to the wrapper or if you use > /proc/self/exe instead. $0 will always contain the full relative path to the wrapper, no? Even if it was found through PATH, the PATH search is done by libc so the shell will be invoked with the full (absolute or relative) path to the wrapper and use that as $0. Or am I missing something? I think the /proc/self/exe thing was something to deal with symlinks, which isn't the issue here. Regards, Arnout
On 09/03/16 11:37, Arnout Vandecappelle wrote: >> Yes, that's basically what I'm saying +/- the question if you want to >> rely on $0 containing the full path to the wrapper or if you use >> /proc/self/exe instead. > > $0 will always contain the full relative path to the wrapper, no? Even > if it was found through PATH, the PATH search is done by libc so the > shell will be invoked with the full (absolute or relative) path to the > wrapper and use that as $0. Or am I missing something? > > I think the /proc/self/exe thing was something to deal with symlinks, > which isn't the issue here. Indeed, take the following example: --- #!/bin/sh TUPLE=powerpc-buildroot-linux-uclibcspe WRAPPER_DIR=$(dirname $0) STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../${TUPLE}/sysroot) echo Invocation: $0 echo Wrapper dir: $WRAPPER_DIR echo Staging dir: $STAGING_DIR --- Run full pathspec /home/gustavoz/b/router03/output/host/usr/bin/ja: Invocation: /home/gustavoz/b/router03/output/host/usr/bin/ja Wrapper dir: /home/gustavoz/b/router03/output/host/usr/bin Staging dir: /home/gustavoz/b/router03/output/host/usr/powerpc-buildroot-linux-uclibcspe/sysroot Run in-place ./ja: Invocation: ./ja Wrapper dir: . Staging dir: /home/gustavoz/b/router03/output/host/usr/powerpc-buildroot-linux-uclibcspe/sysroot We can add a check to see if ${STAGING_DIR} is defined on and avoid (re)setting it when it is. if [ -z ${STAGING_DIR} ]; then magic; fi Regards.
On 03/09/16 15:28, Gustavo Zacarias wrote: > On 09/03/16 11:03, Thomas Petazzoni wrote: > >> What about doing this relatively to the wrapper location? >> >> WRAPPER_DIR=$(dirname $0) >> STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../../../staging/) >> >> and there you are, it is both relocatable and doesn't rely on >> STAGING_DIR being defined in the environment. >> >> Thomas > > Hi. > That won't work, since staging is an absolute symlink, so when moving the SDK > around it will be broken and fail. > You need something like: > > WRAPPER_DIR=$(dirname $0) > STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../@TUPLE@/sysroot) > > And hardcode tuple which isn't likely to change. Not just unlikely to change: if it changes, the sysroot is completely broken. So this seems like the best solution to me. Thanks! Regards, Arnout
On 09/03/16 11:40, Arnout Vandecappelle wrote: > Not just unlikely to change: if it changes, the sysroot is completely > broken. > > So this seems like the best solution to me. Thanks! Yes, that's why i said at the beginning "you're in serious trouble" (if it changes) ;) Regards.
On 03/09/16 15:40, Arnout Vandecappelle wrote: > On 03/09/16 15:28, Gustavo Zacarias wrote: >> On 09/03/16 11:03, Thomas Petazzoni wrote: >> >>> What about doing this relatively to the wrapper location? >>> >>> WRAPPER_DIR=$(dirname $0) >>> STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../../../staging/) >>> >>> and there you are, it is both relocatable and doesn't rely on >>> STAGING_DIR being defined in the environment. >>> >>> Thomas >> >> Hi. >> That won't work, since staging is an absolute symlink, so when moving the SDK >> around it will be broken and fail. >> You need something like: >> >> WRAPPER_DIR=$(dirname $0) >> STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../@TUPLE@/sysroot) >> >> And hardcode tuple which isn't likely to change. > > Not just unlikely to change: if it changes, the sysroot is completely broken. > > So this seems like the best solution to me. Thanks! One small thing: you should use $(STAGING_SUBDIR), which is already defined. Regards, Arnout > > Regards, > Arnout > >
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: > On 03/09/16 15:31, Peter Korsgaard wrote: >>>>>>> "Gustavo" == Gustavo Zacarias <gustavo@zacarias.com.ar> writes: >> >> > On 09/03/16 11:00, Peter Korsgaard wrote: >> >> Well, we cannot really break existing features to add new ones. The >> >> relative location between the pkg-config wrapper script and the staging >> >> directory is constant (or rather known at build time), so I guess we can >> >> do something with /proc/self/exe. We might need to implement the wrapper >> >> in C instead for that to work as /proc/self/exe for a shell script seems >> >> to return /bin/dash here. >> >> > That's overly complicated for no value at all. >> > You know the sysroot is in the host directory as well, so you just >> > need to filter out /usr/bin from $0 and hardcode the tuple in the >> > wrapper - >> > that's guaranteed to not change, otherwise you're in serious trouble. >> >> Yes, that's basically what I'm saying +/- the question if you want to >> rely on $0 containing the full path to the wrapper or if you use >> /proc/self/exe instead. > $0 will always contain the full relative path to the wrapper, no? > Even if it was found through PATH, the PATH search is done by libc so > the shell will be invoked with the full (absolute or relative) path to > the wrapper and use that as $0. Or am I missing something? well, if executed throyh a shell it will - But if I write code to call execve I get to chose what argv[0] should be. Some software (like upx) wants to handle situations like this, but it might not be necessary for our pkg-config / vala wrappers. > I think the /proc/self/exe thing was something to deal with symlinks, > which isn't the issue here. That's another reason. If somebody does a symlink to our wrapper, then $0 will contain the full path to the link, not to the wrapper itself. If somebody does a hardlink, /proc/self/exe won't even fix it for you - But that is probably quite unlikely.
Dear Peter Korsgaard, On Wed, 09 Mar 2016 15:30:18 +0100, Peter Korsgaard wrote: > > WRAPPER_DIR=$(dirname $0) > > That relies on argv[0] containing the full path, which it normally will > do when you run it from the shell, but isn't guaranteed to work. If we > want to support that then we should use /proc/self/exe Well, OK. > > STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../../../staging/) > > This will only work if you keep the entire buildroot output around (or > rather output/staging) and don't customize the BR2_HOST_DIR. Indeed, so we need to do like Arnout suggests, hardcoding the tuple inside the script, so that we really only on the contents of host/, and not on the staging symlink. Best regards, Thomas
On 03/09/16 15:46, Peter Korsgaard wrote: >>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: > > > On 03/09/16 15:31, Peter Korsgaard wrote: > >>>>>>> "Gustavo" == Gustavo Zacarias <gustavo@zacarias.com.ar> writes: > >> > >> > On 09/03/16 11:00, Peter Korsgaard wrote: > >> >> Well, we cannot really break existing features to add new ones. The > >> >> relative location between the pkg-config wrapper script and the staging > >> >> directory is constant (or rather known at build time), so I guess we can > >> >> do something with /proc/self/exe. We might need to implement the wrapper > >> >> in C instead for that to work as /proc/self/exe for a shell script seems > >> >> to return /bin/dash here. > >> > >> > That's overly complicated for no value at all. > >> > You know the sysroot is in the host directory as well, so you just > >> > need to filter out /usr/bin from $0 and hardcode the tuple in the > >> > wrapper - > >> > that's guaranteed to not change, otherwise you're in serious trouble. > >> > >> Yes, that's basically what I'm saying +/- the question if you want to > >> rely on $0 containing the full path to the wrapper or if you use > >> /proc/self/exe instead. > > > $0 will always contain the full relative path to the wrapper, no? > > Even if it was found through PATH, the PATH search is done by libc so > > the shell will be invoked with the full (absolute or relative) path to > > the wrapper and use that as $0. Or am I missing something? > > well, if executed throyh a shell it will - But if I write code to call > execve I get to chose what argv[0] should be. Read the section "Interpreter scripts" of execve(2). argv[0] is discarded. > > Some software (like upx) wants to handle situations like this, but it > might not be necessary for our pkg-config / vala wrappers. > >> I think the /proc/self/exe thing was something to deal with symlinks, > > which isn't the issue here. > > That's another reason. If somebody does a symlink to our wrapper, then > $0 will contain the full path to the link, not to the wrapper > itself. So we need to readlink it first. > If somebody does a hardlink, /proc/self/exe won't even fix it for you - > But that is probably quite unlikely. For the hardlink case there is no solution. Regards, Arnout
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: Hi, >> > STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../../../staging/) >> >> This will only work if you keep the entire buildroot output around (or >> rather output/staging) and don't customize the BR2_HOST_DIR. > Indeed, so we need to do like Arnout suggests, hardcoding the tuple > inside the script, so that we really only on the contents of host/, and > not on the staging symlink. Yes, like I proposed ;)
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: Hi, >> well, if executed throyh a shell it will - But if I write code to call >> execve I get to chose what argv[0] should be. > Read the section "Interpreter scripts" of execve(2). argv[0] is discarded. Ahh yes, for a script this would be true because of the shebang handling in the kernel, but it is not true for normal ELF binaries. >> That's another reason. If somebody does a symlink to our wrapper, then >> $0 will contain the full path to the link, not to the wrapper >> itself. > So we need to readlink it first. Yes, or read /proc/self/exec. >> If somebody does a hardlink, /proc/self/exe won't even fix it for you - >> But that is probably quite unlikely. > For the hardlink case there is no solution. Indeed.
On Wed, Mar 9, 2016 at 3:03 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Wed, 9 Mar 2016 10:51:52 -0300, Gustavo Zacarias wrote: > >> Indeed it does, but you can't have the cake and eat it at once can you? >> Arnout in the famous vala-wrapper thread pointed that we shouldn't >> hardcode the staging directory in it and i've adjusted accordingly, so >> which way is it? (same logic would apply to both cases). Re: >> http://lists.busybox.net/pipermail/buildroot/2016-February/153671.html > > What about doing this relatively to the wrapper location? > > WRAPPER_DIR=$(dirname $0) > STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../../../staging/) > > and there you are, it is both relocatable and doesn't rely on > STAGING_DIR being defined in the environment. Funny that's close to what I've done in my relocatable sdk branch few days ago... https://github.com/tSed/buildroot/commit/cebeb4f43a44eda8e9c1d2fd9629e9f9deea3f28 > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
>>>>> "Samuel" == Samuel Martin <s.martin49@gmail.com> writes: Hi, >> What about doing this relatively to the wrapper location? >> >> WRAPPER_DIR=$(dirname $0) >> STAGING_DIR=$(readlink -f ${WRAPPER_DIR}/../../../staging/) >> >> and there you are, it is both relocatable and doesn't rely on >> STAGING_DIR being defined in the environment. > Funny that's close to what I've done in my relocatable sdk branch few > days ago... > https://github.com/tSed/buildroot/commit/cebeb4f43a44eda8e9c1d2fd9629e9f9deea3f28 Heh, great minds think alike ;) Something else more-or-less related that I think we should fix/workaround until the relocatable sdk rpath handling gets integrated is the issue about the toolchain linking to gmp/mpc/mpfr, and not finding our libraries when the toolchain is moved. That is currently quite painful, especially when you want to reuse the toolchain on different machines/distributions, that may not have compatible libraries in /usr/lib. The easiest workaround is just to build our host-gmp/mpc/mpfr statically for now. This has minimal size impact on the toolchain (< 1MB for a .tar.gz of output/host).
diff --git a/package/pkgconf/pkg-config.in b/package/pkgconf/pkg-config.in index 4dec487..51ac8c1 100644 --- a/package/pkgconf/pkg-config.in +++ b/package/pkgconf/pkg-config.in @@ -1,2 +1,2 @@ #!/bin/sh -PKG_CONFIG_LIBDIR=${PKG_CONFIG_LIBDIR:-@PKG_CONFIG_LIBDIR@} PKG_CONFIG_SYSROOT_DIR=${PKG_CONFIG_SYSROOT_DIR:-@STAGING_DIR@} $(dirname $0)/pkgconf @STATIC@ $@ +PKG_CONFIG_LIBDIR=${PKG_CONFIG_LIBDIR:-${STAGING_DIR}/usr/lib/pkgconfig:${STAGING_DIR}/usr/share/pkgconfig} PKG_CONFIG_SYSROOT_DIR=${PKG_CONFIG_SYSROOT_DIR:-${STAGING_DIR}} $(dirname $0)/pkgconf @STATIC@ $@ diff --git a/package/pkgconf/pkgconf.mk b/package/pkgconf/pkgconf.mk index c8b0cba..3cfcc2a 100644 --- a/package/pkgconf/pkgconf.mk +++ b/package/pkgconf/pkgconf.mk @@ -19,9 +19,6 @@ endef define HOST_PKGCONF_INSTALL_WRAPPER $(INSTALL) -m 0755 -D package/pkgconf/pkg-config.in \ $(HOST_DIR)/usr/bin/pkg-config - $(SED) 's,@PKG_CONFIG_LIBDIR@,$(STAGING_DIR)/usr/lib/pkgconfig:$(STAGING_DIR)/usr/share/pkgconfig,' \ - -e 's,@STAGING_DIR@,$(STAGING_DIR),' \ - $(HOST_DIR)/usr/bin/pkg-config endef define HOST_PKGCONF_STATIC
Use shell ${STAGING_DIR} to expand at run time instead of build time hence avoiding hardcoding the staging directory in it and making it relocatable. Signed-off-by: Gustavo Zacarias <gustavo@zacarias.com.ar> --- package/pkgconf/pkg-config.in | 2 +- package/pkgconf/pkgconf.mk | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-)