Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

download dependencies in a separate task #149

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

banditopazzo
Copy link
Contributor

@banditopazzo banditopazzo commented Oct 9, 2023

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
  • update documentation

In the meantime, please let me know what you think

Fix #150

@posborne
Copy link
Member

posborne commented Oct 9, 2023

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
Copy link
Member

#150

@banditopazzo
Copy link
Contributor Author

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

@posborne
Copy link
Member

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.

@banditopazzo
Copy link
Contributor Author

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

echo >> ${CARGO_HOME}/config
echo "[build]" >> ${CARGO_HOME}/config
echo "rustflags = ['-C', 'rpath']" >> ${CARGO_HOME}/config

was replace with this:

export CARGO_BUILD_FLAGS="-C rpath"

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:

export RUSTFLAGS="${RUSTFLAGS}"

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 banditopazzo changed the title Enable network by default at compile task Cargo bin class improvements Oct 11, 2023
classes/cargo_bin.bbclass Outdated Show resolved Hide resolved
classes/cargo_bin.bbclass Outdated Show resolved Hide resolved
@benjamin-nw
Copy link

benjamin-nw commented Oct 19, 2023

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.

@banditopazzo
Copy link
Contributor Author

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?

@benjamin-nw
Copy link

benjamin-nw commented Dec 6, 2023

I think that's a good trade-off 👍

@posborne
Copy link
Member

posborne commented Dec 7, 2023

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.

@banditopazzo banditopazzo force-pushed the enable_network_in_compile_task branch from cad67a7 to d7f8bcb Compare May 23, 2024 17:40
@banditopazzo banditopazzo changed the title Cargo bin class improvements download dependencies in a separate task May 23, 2024
@banditopazzo
Copy link
Contributor Author

Rebased and updated documentation

CARGO_BUILD_PROFILE ?= "release"
# This is based on the content of CARGO_BUILD_FLAGS and generally will need to
# change if CARGO_BUILD_FLAGS changes.
CARGO_BUILD_PROFILE ?= "${@oe.utils.conditional('DEBUG_BUILD', '1', 'debug', 'release', d)}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@posborne in my previous changes I added this conditional selection to have a debug build for coherency with the global Yocto settings.

I noticed you made some changes about CARGO_BINDIR, is it correct to use 'debug' or I have to use 'dev'?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split network access (cargo fetch) to do_fetch task and disable network in do_compile
3 participants