| Message ID | 20175_1644422934_6203E716_20175_175_1_fea7d1545892211e01cb6660dc5fba16b0851c47.1644422916.git.yann.morin@orange.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/5,v2] package/pkg-cargo: allow packages to define download environment | expand |
On Wed, 9 Feb 2022 17:08:44 +0100 <yann.morin@orange.com> wrote: > +# If building in a sub directory, use that to find the Cargo.toml, unless > +# the package already provided its location. > +ifneq ($$($(2)_SUBDIR),) > +ifeq ($$(filter BR_CARGO_MANIFEST_PATH=%,$$($(2)_DL_ENV)),) > +$(2)_DL_ENV += BR_CARGO_MANIFEST_PATH=$$($(2)_SUBDIR)/Cargo.toml I find that a bit "meh". Should we have an explicit package variable that tells the location of the Cargo.toml, instead of directly have packages pass this "magic" BR_CARGO_MANIFEST_PATH variable ? Should $(2)_SUBDIR be documented for the cargo-package infra in the documentation ? Thomas
Thomas, All, On 2022-02-09 20:54 +0100, Thomas Petazzoni spake thusly: > On Wed, 9 Feb 2022 17:08:44 +0100 > <yann.morin@orange.com> wrote: > > +# If building in a sub directory, use that to find the Cargo.toml, unless > > +# the package already provided its location. > > +ifneq ($$($(2)_SUBDIR),) > > +ifeq ($$(filter BR_CARGO_MANIFEST_PATH=%,$$($(2)_DL_ENV)),) > > +$(2)_DL_ENV += BR_CARGO_MANIFEST_PATH=$$($(2)_SUBDIR)/Cargo.toml > > I find that a bit "meh". I too am not fond of it, to be honest... But I am not a rust/cargo expert, by far, I wanted not to break any existing setup. However, the cargo infra is brand new, and BR_CARGO_MANIFEST_PATH was not even advertised either, so we should probably not have to expect any package to actually use it already. > Should we have an explicit package variable > that tells the location of the Cargo.toml, instead of directly have > packages pass this "magic" BR_CARGO_MANIFEST_PATH variable ? I was wondering if that would even make sense to have a different _SUBDIR and BR_CARGO_MANIFEST_PATH to begin with? We have a package here that seems to be in such a situation, though: https://github.com/Orange-OpenSource/its-client The Cargo.toml is in rust/ but we need to do the build in rust/its-client/ ("cargo build" works perfectly well with a virtual workspace, like is used here, but "cargo install" refuses to work, muahaha...) Still, because of a missing Cargo.lock in that package, vendoring by Buildroot does not work eother (we'll fix that later). So, for now, I suggest we just expect that BR_CARGO_MANIFEST_PATH is the same as _SUBDIR, and thus the conditional assignment is not needed. If in the future we do have an actual, working situation where they differ, then we may add the necessary infra, seems like a plan? In the meantime, I'll respin the series with the ugly conditional removed. > Should $(2)_SUBDIR be documented for the cargo-package infra in the > documentation ? It is not documented for any of the other infras that make use of it (autotools, cmake, python, etc...). The only mention of _SUBDIR in the manual is about _SUBDIRS (plural) for the kernel-module sub-infra. Thanks for the review! Regards, Yann E. MORIN. > Thomas > -- > Thomas Petazzoni, co-owner and CEO, Bootlin > Embedded Linux and Kernel engineering and training > https://bootlin.com
Thomas, Alll, On 2022-02-10 16:08 +0100, MORIN Yann INNOV/IT-S spake thusly: > On 2022-02-09 20:54 +0100, Thomas Petazzoni spake thusly: > > Should $(2)_SUBDIR be documented for the cargo-package infra in the > > documentation ? > It is not documented for any of the other infras that make use of it > (autotools, cmake, python, etc...). > The only mention of _SUBDIR in the manual is about _SUBDIRS (plural) for > the kernel-module sub-infra. Scratch that, I seem to be unable to do a proper search in a webpage... :-( I'll add it to the cargo part of the manual. Cordialement, Yann E. MORIN.
diff --git a/package/pkg-cargo.mk b/package/pkg-cargo.mk index 66bea513e0..e1d81197b5 100644 --- a/package/pkg-cargo.mk +++ b/package/pkg-cargo.mk @@ -71,6 +71,14 @@ $(2)_DOWNLOAD_DEPENDENCIES += host-rustc $(2)_DOWNLOAD_POST_PROCESS = cargo $(2)_DL_ENV += CARGO_HOME=$$(HOST_DIR)/share/cargo +# If building in a sub directory, use that to find the Cargo.toml, unless +# the package already provided its location. +ifneq ($$($(2)_SUBDIR),) +ifeq ($$(filter BR_CARGO_MANIFEST_PATH=%,$$($(2)_DL_ENV)),) +$(2)_DL_ENV += BR_CARGO_MANIFEST_PATH=$$($(2)_SUBDIR)/Cargo.toml +endif +endif + # Due to vendoring, it is pretty likely that not all licenses are # listed in <pkg>_LICENSE. $(2)_LICENSE += , vendored dependencies licenses probably not listed @@ -97,7 +105,7 @@ $(2)_LICENSE += , vendored dependencies licenses probably not listed ifndef $(2)_BUILD_CMDS ifeq ($(4),target) define $(2)_BUILD_CMDS - cd $$(@D) && \ + cd $$($$(PKG)_SRCDIR) && \ $$(TARGET_MAKE_ENV) \ $$(TARGET_CONFIGURE_OPTS) \ $$(PKG_CARGO_ENV) \ @@ -111,7 +119,7 @@ define $(2)_BUILD_CMDS endef else # ifeq ($(4),target) define $(2)_BUILD_CMDS - cd $$(@D) && \ + cd $$($$(PKG)_SRCDIR) && \ $$(HOST_MAKE_ENV) \ RUSTFLAGS="$$(addprefix -C link-args=,$$(HOST_LDFLAGS))" \ $$(HOST_CONFIGURE_OPTS) \ @@ -133,7 +141,7 @@ endif # ifndef $(2)_BUILD_CMDS # ifndef $(2)_INSTALL_TARGET_CMDS define $(2)_INSTALL_TARGET_CMDS - cd $$(@D) && \ + cd $$($$(PKG)_SRCDIR) && \ $$(TARGET_MAKE_ENV) \ $$(TARGET_CONFIGURE_OPTS) \ $$(PKG_CARGO_ENV) \ @@ -152,7 +160,7 @@ endif ifndef $(2)_INSTALL_CMDS define $(2)_INSTALL_CMDS - cd $$(@D) && \ + cd $$($$(PKG)_SRCDIR) && \ $$(HOST_MAKE_ENV) \ RUSTFLAGS="$$(addprefix -C link-args=,$$(HOST_LDFLAGS))" \ $$(HOST_CONFIGURE_OPTS) \