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

rust-qoriq: Migrate from native to toolchain/syno-qoriq-6.2.4-rust #6123

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

th0ma7
Copy link
Contributor

@th0ma7 th0ma7 commented Jun 4, 2024

Description

rust-qoriq: Migrate from native to toolchain/syno-qoriq-6.2.4-rust

Fixes #6054 and relates to #6107 (comment)

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

@th0ma7 th0ma7 requested a review from hgy59 June 4, 2024 00:11
@th0ma7 th0ma7 self-assigned this Jun 4, 2024
@th0ma7
Copy link
Contributor Author

th0ma7 commented Jun 4, 2024

@hgy59 initial testing seems ok, your take on it would be appreciated (and you may have another more interesting idea).

Copy link
Contributor

@hgy59 hgy59 left a comment

Choose a reason for hiding this comment

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

LGTM

@th0ma7 th0ma7 merged commit 3174f7b into SynoCommunity:master Jun 4, 2024
17 checks passed
@th0ma7 th0ma7 deleted the rust-qoriq-fix branch June 4, 2024 00:32
@th0ma7 th0ma7 mentioned this pull request Jun 4, 2024
10 tasks
@hgy59
Copy link
Contributor

hgy59 commented Jun 8, 2024

@th0ma7, @Safihre
As found in #6107 (comment) there is a github cache issue.

It could be either a toolchain cache or a distrib cache issue.

toolchain cache

IMHO we must move qoriq rust back to native or to another folder than toolchain.

At least we must make syno-qoriq-6.2.4-rust independent whether syno-qoriq-6.2.4 is already build or not.
The current cache implementation is not able to create different caches for syno-qoriq-6.2.4 and syno-qoriq-6.2.4-rust.

from build.yml:

      - name: Cache toolchains
        uses: actions/cache@v4
        with:
          path: toolchain/*/work
          key: toolchain-${{ matrix.arch }}-v3-${{ hashFiles('toolchain/*/digests') }}

The build of syno-qoriq-6.2.4-rust must not create output that matches toolchain/*/work.

If we want to create a cache for the qoriq rust we have the following options

  • move back to native and create a cache for native/rust-qoriq/native-work
  • use a folder different to work for the qoriq rust under toolchain

distrib cache

Is did no futher analysis, but probably we have to exclude distrib/rustup from the distrib cache.

The current distrib cache file is ~250MB and I guess it is a problem that it contains subfolders that we don't want to cache.
IMHO we only want to cache what is downloaded in the "Prepare for Build" action to the top level of distrib folder

we should not cache the sub folders

  • toolchain (the toolchain sources are not needed, since we cache the toolchain work folders)
  • toolkit (we could create additional caches for toolkit/*/work)
  • cargo
  • go
  • nuget
  • pip
  • rustup
  • ...

Since the distrib cache is created for each github workflow, it size matters.
I may be wrong and the distrib cache does not contain the subfolders since those are empty at cache creation time...
(don't know how to look into a cache file)

NB: The distrib cache was introduced to cache the downloaded sources for all jobs within a workflow.
So it must not contain any arch specific files.

@hgy59
Copy link
Contributor

hgy59 commented Jun 8, 2024

The trigger for the comment above was the failing build of #6127 in https://github.com/SynoCommunity/spksrc/actions/runs/9419013785

locally I can run make in syno-qoriq-6.2.4-rust and the build for qoriq succeeds.

@hgy59
Copy link
Contributor

hgy59 commented Jun 8, 2024

(don't know how to look into a cache file)

reading this cli/cli#9125 it seems not possible yet

@th0ma7
Copy link
Contributor Author

th0ma7 commented Jun 8, 2024

Actually the installation of syno-qoriq-6.2.4-rust goes under a work-native sub-directory which would then explains why it isn't cached, thus mismatching with the state of the regular toolchain. But I believe the problem lies elsewhere: it's not possible to confirm that distrib/cargo|rustup cache will match the state of toolchain/* cache.

Unless I miss something, the only way I can think of to permanently solve this would be to always install the rust toolchain under it's corresponding arch toolchain folder such as in this case toolchain/syno-qoriq-6.2.4/work/rust/cargo|rustup.

Otherwise when ever we end-up with a cookie file toolchain/syno-qoriq-6.2.4/work/.rustc_done, it's corresponding cache must match which mean that it assumes that distrib/cargo|rustup exists along with where ever rust-qoriq is located.

Going back to your two suggestions above:

  1. moving back to native will not solve the issue as its state may vary (and actually, native could probably be added to the caches btw but that unrelated).
  2. that's already the case and it doesn't work (i.e. different work directory under toolchain).

Proposing a different approach:

  1. For a start, we could force syno-qoriq-6.2.4-rust to install under its own work instead of work-native OR add work-native sub-folder to the caching of toolchain in the github-action scripts.
  2. If that doesn't work then the option would be to always install rust under its related toolchain.

@hgy59 would you agree with my presumptions above? I'll give it a day to further think about a solution, but maybe a new idea will emerge from this thread...

@hgy59
Copy link
Contributor

hgy59 commented Jun 8, 2024

@th0ma7 I can't remember the motivation to move from native to toolchain.

I still am missing a solution for #6054

  • we must build rust only when building rust packages
  • we must not install rust when installing the qoriq toolchain, but we must ensure the toolchain is installed when building native/rust-qoriq

The only solution I can see is to move back to native and fix the dependencies

@th0ma7
Copy link
Contributor Author

th0ma7 commented Jun 10, 2024

I can't remember the motivation to move from native to toolchain.

This was in order to solve the caching issue. Clearly it didn't for reasons above.

I still am missing a solution for #6054

I was hoping this would have solved it.

  • we must build rust only when building rust packages

That is a totally different approach than current. cargo + rust is a complex beast to setup with the following being defined in tc_vars.mk (took a epyc7002 but it's similar for all):

TC_ENV += CARGO_HOME="/home/spksrc/kernel-fix/spksrc/distrib/cargo"
TC_ENV += RUSTUP_HOME="/home/spksrc/kernel-fix/spksrc/distrib/rustup"
TC_ENV += RUSTUP_TOOLCHAIN="stable"
TC_ENV += CARGO_BUILD_TARGET="x86_64-unknown-linux-gnu"
TC_ENV += CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_AR="/home/spksrc/kernel-fix/spksrc/toolchain/syno-epyc7002-7.2/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-ar"
TC_ENV += CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER="/home/spksrc/kernel-fix/spksrc/toolchain/syno-epyc7002-7.2/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-gcc"
TC_ENV += CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS=" $(ADDITIONAL_RUSTFLAGS)"

It also install the generic host cargo + rust, and then the platform specific toolchain. Considering how target specific this is (in comparison with cmake or host python from native, I thought it was way better to have it installed by default for each archs so the entire toolchain is already there before the build starts. Further as it is more and more needed and pretty much mandatory for python's crossenv.

  • we must not install rust when installing the qoriq toolchain, but we must ensure the toolchain is installed when building native/rust-qoriq

I'm not sure how that can be done? This would mean something like:

  1. converting all rust install from spksrc.tc-rust.mk and toolchain variable export into something else. Issue will be ensuring that variables are made available under each MAKELEVEL, necessitating a hook of somesort.
  2. ensuring that it gets called from spksrc.rust.mk + spksrc.python*.mk ...

Or, just modifying its installation so rust always gets installed under toolchain/syno-<arch>-<version>/work/rust instead of distrib. Although this will increase total disk usage as the host library and tools would get reinstalled on a per arch basis, unless there is a way to keep arch toolchains under a different directory from the host tools and libraries.

The only solution I can see is to move back to native and fix the dependencies

So what about, simple, we disable caching of toolchains all together. rust-qoriq can go back to native, problem solved?

@hgy59 hgy59 mentioned this pull request Jun 10, 2024
6 tasks
@hgy59
Copy link
Contributor

hgy59 commented Jun 10, 2024

@th0ma7 I am trying to fix the caching in #6133

The real problem is, that syno-qoriq-6.2.4-rust installs the rust target into the RUSTUP_HOME folder and not to the (cached) work-native folder.
So far all toolchain caches contain the work folder only, i.e. the extracted toolchain file.

For me it seems impossible to create a qoriq-rust cache that contains not only the extracted files but also the rustup "folder" for qoirq, my (final ?) try will be to exclude toolchain caching for the qoriq arch.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Jun 10, 2024

It is installed under the work-native then uses the rustup command which creates a symlink from the RUST_HOME to the installed directory.

Issues are:

  • should use work instead of work-native so it does get cached under toolchain directory
  • as it creates a direct relationship between the two caches (distrib and toolchain), there will always be a chance that one's state does not match the other. Main issue being the trigger falling from the toolchain .rust_done which implies that the rust toolchains are really installed under the distrib folder.

@hgy59
Copy link
Contributor

hgy59 commented Jun 10, 2024

It is installed under the work-native then uses the rustup command which creates a symlink from the RUST_HOME to the installed directory.

This will definitifly not work, since the symlink will not be restored when reusing the cache.

I now have a working version by disabling the toolchain cache for qoriq.

I run the final tests without caches and then will trigger the build again.

@hgy59
Copy link
Contributor

hgy59 commented Jun 10, 2024

  • as it creates a direct relationship between the two caches (distrib and toolchain), there will always be a chance that one's state does not match the other.

No, we could have one cache for both (qoriq and qoriq-rust) with a key build from both digests file, or we have two different caches with different keys.
The keys will never change (except we update the qoriq-rust sources) and the build state will never trigger a cache update.

IMHO the distrib cache does not contain subfolders (rustup, cargo, pip, ...) since it is created after the prepare job, that only downloads source files.
The distrib cache has a different context and must never be combined with toolchain caches.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Jun 10, 2024

I now have a working version by disabling the toolchain cache for qoriq.

Excellent idea. That should solve all cases as no state cookie will then tell otherwise. I would then suggest migrating qoriq-rust back to native.

IMHO the distrib cache does not contain subfolders (rustup, cargo, pip, ...) since it is created after the prepare job, that only downloads source files.

I had not caught this. Still, would have been nice to cache rustup and cargo. Also, pip cache is downloaded like any other package so in theory it is being cached I would assume.

@hgy59
Copy link
Contributor

hgy59 commented Jun 15, 2024

@th0ma7 please consider the following issue, when updating the rust integration.

We currently can not specify a specific rust version for the build of a package (this was originally designed by defintion of RUSTUP_DEFAULT_TOOLCHAIN in a cross/*/Makefile)
You updated spksrc.cross-rust-env.mk to use rust 1.77.2 for ARMv5 archs.
But this is applied at rust installation only (i.e. when building the toolchain or deleting the rustc done cookie).

Currently cross/fd_8.7.0 fails to build for hi2525-6.2.4 with rust 1.79.0
It is a problem with the dependent rust binutils package.
(see build of #6143 in https://github.com/SynoCommunity/spksrc/actions/runs/9520367767/job/26245897404)

if I define RUSTUP_DEFAULT_TOOLCHAIN = 1.78 in cross/fd_8.7.0/Makefile there is no call of rustup update 1.78 during the build.
It even could be required to have different rust versions for different archs of the same package.

Since we have the synocli packages with different tools, it must be possible to use different rust versions within a single spk package.
Some time ago, we had a package that depended on rust nightly. Since we do not want to build all packages with this version, the rust version must depend on packages too.

IMHO it will only work when rustup update is called everytime when a cross package is built with rust.

So, before trying to solve other rust issues, we have to solve this.
And I'm still in favor of decoupling the rust installation from the toolchain installation.

If we can't change the rust version for each cross package using rust, we have to install rust for each such build.
And this will solve the issue, that nobody wants to install rust, when building a package that does not need rust at all...

@th0ma7
Copy link
Contributor Author

th0ma7 commented Jun 16, 2024

IMHO it will only work when rustup update is called everytime when a cross package is built with rust.

to have per-cross granularity, yet.

So, before trying to solve other rust issues, we have to solve this. And I'm still in favor of decoupling the rust installation from the toolchain installation.

I am not against, and one doesn't prevent the other neither, i.e. this doesn't mean that it cannot use the pre-installed rust host tools to install any new version needed and use for a specific build. Still code needs to be adapted.

If we can't change the rust version for each cross package using rust, we have to install rust for each such build. And this will solve the issue, that nobody wants to install rust, when building a package that does not need rust at all...

This isn't going to be easy... how to manage qoriq or armv5 will end-up being a real pain unless they simply get discarded for those use-cases. food for thoughts.

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.

missing native/rust-qoriq dependency for qoriq
2 participants