| Message ID | 1443456634-21484-3-git-send-email-luca@lucaceresoli.net |
|---|---|
| State | Superseded |
| Headers | show |
Hi Luca, On Mon, Sep 28, 2015 at 06:10:32PM +0200, Luca Ceresoli wrote: > A trailing slash in FOO_SITE is useless, since Buildroot automatically adds > a slash between FOO_SITE and the filename as appropriate. > > Moreover they are potentially armful, which led to introducing a workaround harmful? > to strip them: baruch
Dear Baruch, Baruch Siach wrote: > Hi Luca, > > On Mon, Sep 28, 2015 at 06:10:32PM +0200, Luca Ceresoli wrote: >> A trailing slash in FOO_SITE is useless, since Buildroot automatically adds >> a slash between FOO_SITE and the filename as appropriate. >> >> Moreover they are potentially armful, which led to introducing a workaround > > harmful? Of course! Thanks for spotting it.
Dear Luca Ceresoli, On Mon, 28 Sep 2015 18:10:32 +0200, Luca Ceresoli wrote: > # Retrieve the archive > $(BUILD_DIR)/%/.stamp_downloaded: > + @(echo "$($(PKG)_SITE)" | grep -Eq "[^/]$$" || \ > + (echo "$(PKG)_SITE ($($(PKG)_SITE)) cannot have a trailing slash" && false)) You could use $(error ...) instead: > + @(echo "$($(PKG)_SITE)" | grep -Eq "[^/]$$" || \ > + $(error "$(PKG)_SITE ($($(PKG)_SITE)) cannot have a trailing slash")) I am wondering if there are indeed no places left where a final / could sneak in. For example, when you specify a custom tarball location for packages like U-Boot, the Linux kernel and so on. For Linux and U-Boot, it is taken care of: UBOOT_SITE = $(patsubst %/,%,$(dir $(UBOOT_TARBALL))) LINUX_SITE = $(patsubst %/,%,$(dir $(LINUX_TARBALL))) But for example, for the external toolchain site: TOOLCHAIN_EXTERNAL_SITE = $(dir $(call qstrip,$(BR2_TOOLCHAIN_EXTERNAL_URL))) I believe this means that the TOOLCHAIN_EXTERNAL_SITE variable will contain a final /. But that's indeed the only case I could find from a quick inspection. Best regards, Thomas
On 30-09-15 11:45, Thomas Petazzoni wrote: > Dear Luca Ceresoli, > > On Mon, 28 Sep 2015 18:10:32 +0200, Luca Ceresoli wrote: > >> # Retrieve the archive >> $(BUILD_DIR)/%/.stamp_downloaded: >> + @(echo "$($(PKG)_SITE)" | grep -Eq "[^/]$$" || \ >> + (echo "$(PKG)_SITE ($($(PKG)_SITE)) cannot have a trailing slash" && false)) > > You could use $(error ...) instead: > >> + @(echo "$($(PKG)_SITE)" | grep -Eq "[^/]$$" || \ >> + $(error "$(PKG)_SITE ($($(PKG)_SITE)) cannot have a trailing slash")) That's not going to work AFAIK. The whole thing would have to be converted to a make condition. BTW, Luca, you should indicate in your commit message why this has to be part of .stamp_downloaded and not a static check in inner-generic-package. > > I am wondering if there are indeed no places left where a final / could > sneak in. For example, when you specify a custom tarball location for > packages like U-Boot, the Linux kernel and so on. > > For Linux and U-Boot, it is taken care of: > > UBOOT_SITE = $(patsubst %/,%,$(dir $(UBOOT_TARBALL))) > > LINUX_SITE = $(patsubst %/,%,$(dir $(LINUX_TARBALL))) > > But for example, for the external toolchain site: > > TOOLCHAIN_EXTERNAL_SITE = $(dir $(call qstrip,$(BR2_TOOLCHAIN_EXTERNAL_URL))) > > I believe this means that the TOOLCHAIN_EXTERNAL_SITE variable will > contain a final /. But that's indeed the only case I could find from a > quick inspection. Well, to be consistent, the removal should not be done for U-Boot or Linux either, and instead the user should get this error message when he builds. Regards, Arnout > > Best regards, > > Thomas >
On 03-10-15 13:01, Arnout Vandecappelle wrote: > On 30-09-15 11:45, Thomas Petazzoni wrote: >> > Dear Luca Ceresoli, >> > >> > On Mon, 28 Sep 2015 18:10:32 +0200, Luca Ceresoli wrote: >> > >>> >> # Retrieve the archive >>> >> $(BUILD_DIR)/%/.stamp_downloaded: >>> >> + @(echo "$($(PKG)_SITE)" | grep -Eq "[^/]$$" || \ >>> >> + (echo "$(PKG)_SITE ($($(PKG)_SITE)) cannot have a trailing slash" && false)) >> > >> > You could use $(error ...) instead: >> > >>> >> + @(echo "$($(PKG)_SITE)" | grep -Eq "[^/]$$" || \ >>> >> + $(error "$(PKG)_SITE ($($(PKG)_SITE)) cannot have a trailing slash")) > That's not going to work AFAIK. The whole thing would have to be converted to a > make condition. E.g.: ifeq ($(patsubst %/,ERROR,$($(PKG)_SITE)),ERROR) $(error ...) endif Or if that doesn't work: ifeq ($(shell echo "$($(PKG)_SITE)" | grep -Eq "[^/]$$" || echo n),n) $(error ...) endif Regards, Arnout
Hi Arnout, Thomas, Arnout Vandecappelle wrote: [...] > >> >> I am wondering if there are indeed no places left where a final / could >> sneak in. For example, when you specify a custom tarball location for >> packages like U-Boot, the Linux kernel and so on. >> >> For Linux and U-Boot, it is taken care of: >> >> UBOOT_SITE = $(patsubst %/,%,$(dir $(UBOOT_TARBALL))) >> >> LINUX_SITE = $(patsubst %/,%,$(dir $(LINUX_TARBALL))) >> >> But for example, for the external toolchain site: >> >> TOOLCHAIN_EXTERNAL_SITE = $(dir $(call qstrip,$(BR2_TOOLCHAIN_EXTERNAL_URL))) >> >> I believe this means that the TOOLCHAIN_EXTERNAL_SITE variable will >> contain a final /. But that's indeed the only case I could find from a >> quick inspection. > > Well, to be consistent, the removal should not be done for U-Boot or Linux > either, and instead the user should get this error message when he builds. But FOO_SITE is generated internally when the user chooses to fetch from a custom URL. The user has no access to FOO_SITE in these cases, so Buildroot should strip them. So I guess I'll add a patsubst to toolchain-external too, and hunt for other places where it might be needed.
On 03-10-15 16:07, Luca Ceresoli wrote: > Hi Arnout, Thomas, > > Arnout Vandecappelle wrote: > [...] >> >>> >>> I am wondering if there are indeed no places left where a final / could >>> sneak in. For example, when you specify a custom tarball location for >>> packages like U-Boot, the Linux kernel and so on. >>> >>> For Linux and U-Boot, it is taken care of: >>> >>> UBOOT_SITE = $(patsubst %/,%,$(dir $(UBOOT_TARBALL))) >>> >>> LINUX_SITE = $(patsubst %/,%,$(dir $(LINUX_TARBALL))) >>> >>> But for example, for the external toolchain site: >>> >>> TOOLCHAIN_EXTERNAL_SITE = $(dir $(call qstrip,$(BR2_TOOLCHAIN_EXTERNAL_URL))) >>> >>> I believe this means that the TOOLCHAIN_EXTERNAL_SITE variable will >>> contain a final /. But that's indeed the only case I could find from a >>> quick inspection. >> >> Well, to be consistent, the removal should not be done for U-Boot or Linux >> either, and instead the user should get this error message when he builds. > > But FOO_SITE is generated internally when the user chooses to fetch from > a custom URL. The user has no access to FOO_SITE in these cases, so > Buildroot should strip them. You're completely right, sorry! > So I guess I'll add a patsubst to toolchain-external too, and hunt for > other places where it might be needed. I can't think of an easy grep for that :-( Regards, Arnout
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 5201fca..73353aa 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -70,6 +70,8 @@ endif # Retrieve the archive $(BUILD_DIR)/%/.stamp_downloaded: + @(echo "$($(PKG)_SITE)" | grep -Eq "[^/]$$" || \ + (echo "$(PKG)_SITE ($($(PKG)_SITE)) cannot have a trailing slash" && false)) $(foreach hook,$($(PKG)_PRE_DOWNLOAD_HOOKS),$(call $(hook))$(sep)) # Only show the download message if it isn't already downloaded $(Q)for p in $($(PKG)_ALL_DOWNLOADS); do \
A trailing slash in FOO_SITE is useless, since Buildroot automatically adds a slash between FOO_SITE and the filename as appropriate. Moreover they are potentially armful, which led to introducing a workaround to strip them: commit 1cbffbd015106ea90fe49e27433375769dc1035b Author: Shawn J. Goff <shawn7400@gmail.com> Date: Fri Apr 12 09:40:30 2013 +0000 eliminate double slashes caused by FOO_SITE ending in a slash When a FOO_SITE variable ends in a slash and gets joined with a FOO_SOURCE variable like $(FOO_SITE)/$(FOO_SOURCE), the resulting URI has a double slash. While double-slashes are fine in unix paths, they are reserved in URIs - the part following '//' must be an authority. So let's ban trailing slashes entirely. They have all been removed in a recent commit, now add a check to error out loudly in case a new one is added. Example commands to test this check: $ make busybox-dirclean busybox-source rm -Rf /home/murray/devel/buildroot/output/build/busybox-1.23.2 busybox-1.23.2.tar.bz2: OK (md5: 7925683d7dd105aabe9b6b618d48cc73) busybox-1.23.2.tar.bz2: OK (sha1: 7f37193cb249f27630e0b2a2c6c9bbb7b1d24c16) $ $ make BUSYBOX_SITE=http://www.busybox.net/downloads/ busybox-dirclean busybox-source rm -Rf /home/murray/devel/buildroot/output/build/busybox-1.23.2 BUSYBOX_SITE (http://www.busybox.net/downloads/) cannot have a trailing slash make[1]: *** [/home/murray/devel/buildroot/output/build/busybox-1.23.2/.stamp_downloaded] Error 1 make: *** [_all] Error 2 $ Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> --- Changed v2 -> v3: - Simplify the check logic removing a useless "&& true". - Expand commit log message. This is a pretty important change, so it should be well justified. --- package/pkg-generic.mk | 2 ++ 1 file changed, 2 insertions(+)