meta-rust-bin icon indicating copy to clipboard operation
meta-rust-bin copied to clipboard

download dependencies in a separate task

Open banditopazzo opened this issue 2 years ago • 10 comments

I would like to enable the network during the compile task by default on cargo_bin class.

I think there is no reason to leave this configuration to the user, this is the normal behaviour expected for cargo_bin.

To finish the PR I need to:

  • [ ] test with both older and newer versions of Yocto
  • [x] update documentation

In the meantime, please let me know what you think

Fix #150

banditopazzo avatar Oct 09 '23 10:10 banditopazzo

I agree this makes sense, I'm guessing many users may just be on older Yocto versions or not running on systems that have the requisite kernel support to enforce this.

I think the alternative, and possibly better, alternative would be to run do_compile with cargo build --offline and move the cargo fetch to be performed in do_fetch.

posborne avatar Oct 09 '23 20:10 posborne

https://github.com/rust-embedded/meta-rust-bin/issues/150

posborne avatar Oct 10 '23 16:10 posborne

I agree with the dependency download in do_fetch because it's more Yocto style . The only thing that worries me is if the build.rs of the application or of one of its dependencies interacts with the network

banditopazzo avatar Oct 10 '23 17:10 banditopazzo

I agree with the dependency download in do_fetch because it's more Yocto style . The only thing that worries me is if the build.rs of the application or of one of its dependencies interacts with the network

While that's possible, I don't think it should be enough of a deterrent to safely keep the default as prohibiting network access. Build.rs scripts that reach out to the network are, as far as I know, not generally common or encouraged and likely work against the aims we should aim for by default to have reproducible builds given the same source input.

If this is something that some users encounter, then they can definitely just enable network for do_compile.

posborne avatar Oct 10 '23 18:10 posborne

Working on this I found a problem with the latest PR, this:

https://github.com/rust-embedded/meta-rust-bin/blob/7ac87ecf6c05252b5070c9f6733cff01819f998a/classes/cargo_bin.bbclass#L69-L71

was replace with this:

https://github.com/rust-embedded/meta-rust-bin/blob/2089ae68d3618d351119bf93b2cfa1763ad00224/classes/cargo_bin.bbclass#L106

But it should be CARGO_BUILD_RUSTFLAGS.

However there is another issue. according to the cargo documentation here:

There are four mutually **exclusive** sources of extra flags. They are checked in order, with the first one being used:

1. CARGO_ENCODED_RUSTFLAGS environment variable.
2. RUSTFLAGS environment variable.
3. All matching target.<triple>.rustflags and target.<cfg>.rustflags config entries joined together.
4. build.rustflags config value.

But in the cargo_bin_do_compile there was a definition of RUSTFLAGS, even before the latest PR:

https://github.com/rust-embedded/meta-rust-bin/blob/2089ae68d3618d351119bf93b2cfa1763ad00224/classes/cargo_bin.bbclass#L94

I tested the behaviour and I can confirm that the flag -C rpath was ignored by cargo build.

However for curiosity I tried to enable it manually to understand the reasons behind it and what I get in the ELF is:

0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../../../../recipe-sysroot-native/usr/lib/rustlib/aarch64-unknown-linux-gnu/lib]

This make no sense to me. I think it's safe to remove it

banditopazzo avatar Oct 11 '23 14:10 banditopazzo

Hello,

I like the idea of removing the network phase from the do_compile step and moving it to the do_fetch. It's the Yocto way.

I agree with @posborne on the build.rs accessing the network, it should not be the default and we can let the user enable the do_compile[network] = "1" if ever needed.

benjamin-nw avatar Oct 19 '23 10:10 benjamin-nw

Testing the new class, I discovered that is impossible to call cargo fetch in the do_fetch task because cargo is not present in the binary PATH.

The task order is the following:

$ cat build/tmp/work/core2-64-poky-linux/gpio-utils/1.0-r0/temp/log.task_order 
do_deploy_source_date_epoch_setscene (357874): log.do_deploy_source_date_epoch_setscene.357874
do_fetch (357928): log.do_fetch.357928
do_unpack (357934): log.do_unpack.357934
do_prepare_recipe_sysroot (357935): log.do_prepare_recipe_sysroot.357935
do_patch (357965): log.do_patch.357965
do_configure (357975): log.do_configure.357975
do_compile (357988): log.do_compile.357988
do_compile (358044): log.do_compile.358044
do_compile (358098): log.do_compile.358098
do_compile (358156): log.do_compile.358156

from what I understand cargo executable is available after do_prepare_recipe_sysroot.

recipe tasks in Yocto are not a true standard. for example the kernel has a complete different flow, something like that:

do_fetch
do_unpack
do_prepare_recipe_sysroot
do_kernel_checkout
do_symlink_kernsrc
do_validate_branches
do_kernel_metadata
do_patch
do_kernel_version_sanity_check
do_populate_lic
do_kernel_configme
do_configure
do_kernel_configcheck
do_compile
do_shared_workdir
do_kernel_link_images
do_compile_kernelmodules
do_strip
do_sizecheck
do_install
do_populate_sysroot
do_package
do_packagedata
do_package_qa
do_package_write_ipk
do_bundle_initramfs
do_deploy

my proposal it add a new task called for example do_cargo_fetch like this:

addtask do_cargo_fetch after do_configure before do_compile
do_cargo_fetch[network] = "1"
do_cargo_fetch[dirs]= "${B}"
cargo_bin_do_cargo_fetch() {
    cargo fetch --manifest-path ${CARGO_MANIFEST_PATH}
}

what do you think about it?

banditopazzo avatar Nov 07 '23 16:11 banditopazzo

I think that's a good trade-off :+1:

benjamin-nw avatar Dec 06 '23 16:12 benjamin-nw

my proposal it add a new task called for example do_cargo_fetch like this:

That seems like a pretty reasonable approach to me, agree that the tasks are a convention but it is not unusual to need to define custom tasks for various software packages or flows as required and it can be preferred to trying to do too much with prepend/append onto other tasks.

posborne avatar Dec 07 '23 17:12 posborne

Rebased and updated documentation

banditopazzo avatar May 23 '24 18:05 banditopazzo