| Message ID | 20230814215233.745573-1-gregorhaas1997@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] package/bmap-tools: add dependency on python-six | expand |
Hello Gregor, On Mon, 14 Aug 2023 14:52:33 -0700 Gregor Haas <gregorhaas1997@gmail.com> wrote: > The core bmaptool program provided by this package depends on python-six to run. > It is normally not an issue to use this program to e.g. finalize target images > since all host dependencies are copied to the same place. However, it seems that > bmaptool is configured (at package install time) to use the current host python > interpreter -- and in the case of per-package builds, this is the interpreter in > the per-package directory. Finally, without the explicit dependency on > python-six, this per-package interpreter will not have the necessary packages. > Therefore, add the required dependencies on python-six to ensure that the > bmaptool program can work correctly > > Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com> > --- > package/bmap-tools/bmap-tools.mk | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/package/bmap-tools/bmap-tools.mk b/package/bmap-tools/bmap-tools.mk > index 32399ca151..350286777c 100644 > --- a/package/bmap-tools/bmap-tools.mk > +++ b/package/bmap-tools/bmap-tools.mk > @@ -9,6 +9,8 @@ BMAP_TOOLS_SITE = $(call github,intel,bmap-tools,v$(BMAP_TOOLS_VERSION)) > BMAP_TOOLS_LICENSE = GPL-2.0 > BMAP_TOOLS_LICENSE_FILES = COPYING > BMAP_TOOLS_SETUP_TYPE = setuptools > +BMAP_TOOLS_DEPENDENCIES += python-six I think this one should not be needed. The "select" in the Config.in should be sufficient. > +HOST_BMAP_TOOLS_DEPENDENCIES += host-python-six For the host package, I can indeed reproduce the problem. But I think the issue really is that in the final $(HOST_DIR), we keep the shebang of the Python interpreter pointing to the per-package directory. Indeed, my reasoning is that otherwise we will have many similar problems with other packages, as this is breaking a fundamental assumption in Buildroot. I'm thinking about something like this: diff --git a/Makefile b/Makefile index f0ff9a1480..00ce64ab15 100644 --- a/Makefile +++ b/Makefile @@ -713,6 +713,15 @@ STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.t host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK) @$(call MESSAGE,"Finalizing host directory") $(call per-package-rsync,$(sort $(PACKAGES)),host,$(HOST_DIR)) +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y) + $(Q)grep --binary-files=without-match -lrZ '$(PER_PACKAGE_DIR)/[^/]\+/host' $(HOST_DIR) \ + |while read -d '' f; do \ + file -b --mime-type "$${f}" | grep -q '^text/' || continue; \ + printf '%s\0' "$${f}"; \ + done \ + |xargs -0 --no-run-if-empty \ + $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/host:$(HOST_DIR):g' +endif .PHONY: staging-finalize staging-finalize: $(STAGING_DIR_SYMLINK) Yann, what do you think? (Logic is taken from package/pkg-generic.mk, which does the conversion from per-package directories of dependencies to the per-package directory of the package being built, the logic here is similar, but we switch from the per-package directories to the global host directory) Best regards, Thomas
Hi Thomas, On Tue, Aug 22, 2023 at 12:47 PM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > I'm thinking about something like this: > > diff --git a/Makefile b/Makefile > index f0ff9a1480..00ce64ab15 100644 > --- a/Makefile > +++ b/Makefile > @@ -713,6 +713,15 @@ STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.t > host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK) > @$(call MESSAGE,"Finalizing host directory") > $(call per-package-rsync,$(sort $(PACKAGES)),host,$(HOST_DIR)) > +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y) > + $(Q)grep --binary-files=without-match -lrZ '$(PER_PACKAGE_DIR)/[^/]\+/host' $(HOST_DIR) \ > + |while read -d '' f; do \ > + file -b --mime-type "$${f}" | grep -q '^text/' || continue; \ > + printf '%s\0' "$${f}"; \ > + done \ > + |xargs -0 --no-run-if-empty \ > + $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/host:$(HOST_DIR):g' > +endif > > .PHONY: staging-finalize > staging-finalize: $(STAGING_DIR_SYMLINK) Just wanted to confirm that this patch also fixes my issue, so I'm okay with this instead of the patch I sent earlier. Thanks, Gregor
Hello Gregor, On Wed, 23 Aug 2023 13:17:09 -0700 Gregor Haas <gregorhaas1997@gmail.com> wrote: > Just wanted to confirm that this patch also fixes my issue, so I'm okay with > this instead of the patch I sent earlier. Thanks for your confirmation. Ideally, I'd like to get feedback from Yann (or other Buildroot maintainers) before formally submitting my proposal. Let's see if I hear from Yann. If I don't hear back within the next few days, I'll submit formally. Best regards, Thomas
Thomas, All, On 2023-08-22 21:47 +0200, Thomas Petazzoni spake thusly: > On Mon, 14 Aug 2023 14:52:33 -0700 > Gregor Haas <gregorhaas1997@gmail.com> wrote: > For the host package, I can indeed reproduce the problem. But I think > the issue really is that in the final $(HOST_DIR), we keep the shebang > of the Python interpreter pointing to the per-package directory. > Indeed, my reasoning is that otherwise we will have many similar > problems with other packages, as this is breaking a fundamental > assumption in Buildroot. > > I'm thinking about something like this: > > diff --git a/Makefile b/Makefile > index f0ff9a1480..00ce64ab15 100644 > --- a/Makefile > +++ b/Makefile > @@ -713,6 +713,15 @@ STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.t > host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK) > @$(call MESSAGE,"Finalizing host directory") > $(call per-package-rsync,$(sort $(PACKAGES)),host,$(HOST_DIR)) > +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y) > + $(Q)grep --binary-files=without-match -lrZ '$(PER_PACKAGE_DIR)/[^/]\+/host' $(HOST_DIR) \ > + |while read -d '' f; do \ > + file -b --mime-type "$${f}" | grep -q '^text/' || continue; \ > + printf '%s\0' "$${f}"; \ > + done \ > + |xargs -0 --no-run-if-empty \ > + $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/host:$(HOST_DIR):g' > +endif > > .PHONY: staging-finalize > staging-finalize: $(STAGING_DIR_SYMLINK) > > Yann, what do you think? (Logic is taken from package/pkg-generic.mk, > which does the conversion from per-package directories of dependencies > to the per-package directory of the package being built, the logic here > is similar, but we switch from the per-package directories to the > global host directory) Then we want to avoid code duplication, why can't we re-use the existing PPD_FIXUP_PATHS macro? It might need a bit of tweaking, though: - first, we only need to fixup paths in $(HOST_DIR), and this is already what we are doing. - second, we only need to fix path for host tools, libs et al., so that they can still run, and paths for staging/ libs et al., so that we can link against. However, that second point is not what we are doing, as we are tweaking everything at the root of the PPD: $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g' Furthermore, staging/ is a sub-directory of host/ and we already account for that as we are only grepping in $(HOST_DIR). But with the sed, we replace a shaloower path than HOST_DIR. So I think we can change the existing PPD_FIXUP_PATHS to: define PPD_FIXUP_PATHS $(Q)grep --binary-files=without-match -lrZ '$(PER_PACKAGE_DIR)/[^/]\+/' $(HOST_DIR) \ |while read -d '' f; do \ file -b --mime-type "$${f}" | grep -q '^text/' || continue; \ printf '%s\0' "$${f}"; \ done \ |xargs -0 --no-run-if-empty \ - $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g' + $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/host/:$(HOST_DIR)/:g' endef And then, we can expand that macro in both contexts: PPD_rsync and target-finalize. Thoughts? Regards, Yann E. MORIN.
Hi all, Just wanted to check if there is still interest in this patch? I've been using Yann's proposal for a few months and it seems to work really well for me, but it would be useful for me to have it in upstream rather than carrying around a local patch. Thanks, Gregor On Thu, Aug 24, 2023 at 12:20 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Thomas, All, > > On 2023-08-22 21:47 +0200, Thomas Petazzoni spake thusly: > > On Mon, 14 Aug 2023 14:52:33 -0700 > > Gregor Haas <gregorhaas1997@gmail.com> wrote: > > For the host package, I can indeed reproduce the problem. But I think > > the issue really is that in the final $(HOST_DIR), we keep the shebang > > of the Python interpreter pointing to the per-package directory. > > Indeed, my reasoning is that otherwise we will have many similar > > problems with other packages, as this is breaking a fundamental > > assumption in Buildroot. > > > > I'm thinking about something like this: > > > > diff --git a/Makefile b/Makefile > > index f0ff9a1480..00ce64ab15 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -713,6 +713,15 @@ STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.t > > host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK) > > @$(call MESSAGE,"Finalizing host directory") > > $(call per-package-rsync,$(sort $(PACKAGES)),host,$(HOST_DIR)) > > +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y) > > + $(Q)grep --binary-files=without-match -lrZ '$(PER_PACKAGE_DIR)/[^/]\+/host' $(HOST_DIR) \ > > + |while read -d '' f; do \ > > + file -b --mime-type "$${f}" | grep -q '^text/' || continue; \ > > + printf '%s\0' "$${f}"; \ > > + done \ > > + |xargs -0 --no-run-if-empty \ > > + $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/host:$(HOST_DIR):g' > > +endif > > > > .PHONY: staging-finalize > > staging-finalize: $(STAGING_DIR_SYMLINK) > > > > Yann, what do you think? (Logic is taken from package/pkg-generic.mk, > > which does the conversion from per-package directories of dependencies > > to the per-package directory of the package being built, the logic here > > is similar, but we switch from the per-package directories to the > > global host directory) > > Then we want to avoid code duplication, why can't we re-use the existing > PPD_FIXUP_PATHS macro? > > It might need a bit of tweaking, though: > > - first, we only need to fixup paths in $(HOST_DIR), and this is > already what we are doing. > > - second, we only need to fix path for host tools, libs et al., so > that they can still run, and paths for staging/ libs et al., so that > we can link against. > > However, that second point is not what we are doing, as we are tweaking > everything at the root of the PPD: > > $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g' > > Furthermore, staging/ is a sub-directory of host/ and we already account > for that as we are only grepping in $(HOST_DIR). But with the sed, we > replace a shaloower path than HOST_DIR. > > So I think we can change the existing PPD_FIXUP_PATHS to: > > define PPD_FIXUP_PATHS > $(Q)grep --binary-files=without-match -lrZ '$(PER_PACKAGE_DIR)/[^/]\+/' $(HOST_DIR) \ > |while read -d '' f; do \ > file -b --mime-type "$${f}" | grep -q '^text/' || continue; \ > printf '%s\0' "$${f}"; \ > done \ > |xargs -0 --no-run-if-empty \ > - $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g' > + $(SED) 's:$(PER_PACKAGE_DIR)/[^/]\+/host/:$(HOST_DIR)/:g' > endef > > And then, we can expand that macro in both contexts: PPD_rsync and > target-finalize. > > 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. | > '------------------------------^-------^------------------^--------------------'
diff --git a/package/bmap-tools/bmap-tools.mk b/package/bmap-tools/bmap-tools.mk index 32399ca151..350286777c 100644 --- a/package/bmap-tools/bmap-tools.mk +++ b/package/bmap-tools/bmap-tools.mk @@ -9,6 +9,8 @@ BMAP_TOOLS_SITE = $(call github,intel,bmap-tools,v$(BMAP_TOOLS_VERSION)) BMAP_TOOLS_LICENSE = GPL-2.0 BMAP_TOOLS_LICENSE_FILES = COPYING BMAP_TOOLS_SETUP_TYPE = setuptools +BMAP_TOOLS_DEPENDENCIES += python-six +HOST_BMAP_TOOLS_DEPENDENCIES += host-python-six $(eval $(python-package)) $(eval $(host-python-package))
The core bmaptool program provided by this package depends on python-six to run. It is normally not an issue to use this program to e.g. finalize target images since all host dependencies are copied to the same place. However, it seems that bmaptool is configured (at package install time) to use the current host python interpreter -- and in the case of per-package builds, this is the interpreter in the per-package directory. Finally, without the explicit dependency on python-six, this per-package interpreter will not have the necessary packages. Therefore, add the required dependencies on python-six to ensure that the bmaptool program can work correctly Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com> --- package/bmap-tools/bmap-tools.mk | 2 ++ 1 file changed, 2 insertions(+)