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

chore: Revamp build system to overcome Cross issues #670

Merged
merged 8 commits into from
Oct 27, 2023

Conversation

diconico07
Copy link
Contributor

What this PR does / why we need it:
This PR completely refactor the build system for akri, aiming to fix #656 :

  • Rust components are now built within docker using XX for cross-compilation
  • Made samples clear in a separate Makefile to ease with them getting out of the main repo soon
  • Remove cross intermediate image
  • Use buildx to build for multiple architectures
  • Merge largely similar Dockerfiles
  • Upgrade opcua-monitoring-broker to .NET 6 (part of Upgrade .Net Core 3.1 binaries to .Net 6 LTS or 7 #608)
  • Upgrade opencv intermediate image to .NET 6 (the onvif broker still needs migration)

Special notes for your reviewer:

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thanks for this amazing revamp! Great to see everything is building.

Makefile Show resolved Hide resolved
.github/workflows/build-rust-code.yml Outdated Show resolved Hide resolved
This commit completely refactor the build system for akri:
 - Rust components are now built within docker using XX for
   cross-compilation
 - Made samples clear in a separate Makefile to ease with them getting
   out of the main repo soon
 - Remove cross intermediate image
 - Use buildx to build for multiple architectures
 - Merge largely similar Dockerfiles
 - Upgrade opcua-monitoring-broker to .NET 6
 - Upgrade opencv intermediate image to .NET 6 (the onvif broker still
   needs migration)

Signed-off-by: Nicolas Belouin <[email protected]>
Signed-off-by: Nicolas Belouin <[email protected]>
Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thank you for revamping this. One ignorable nit on file rename but LGTM!

.github/workflows/build-rust-code.yml Outdated Show resolved Hide resolved
diconico07 and others added 3 commits October 26, 2023 11:48
Also fix the built component always set to agent.

Signed-off-by: Nicolas Belouin <[email protected]>
@bfjelds
Copy link
Collaborator

bfjelds commented Oct 26, 2023

this is an incredible improvement!! super awesome.

from a quick glance, it looks like build times increase a bit ... if that's correct, i just want to make sure that isn't a problem for anything (this may be an outdated recollection, but i have vague memories about the test pipeline having a sleep to wait for some build part to finish: here).

maybe some of the .github/actions code can be removed as well?

@diconico07
Copy link
Contributor Author

from a quick glance, it looks like build times increase a bit ... if that's correct, i just want to make sure that isn't a problem for anything (this may be an outdated recollection, but i have vague memories about the test pipeline having a sleep to wait for some build part to finish: here).

I increased the sleep you mentioned to be the maximum allowed run time for the build job, this way we are sure it won't cause any issue (also increased maximum run time for the run-test-cases job to take this change into account).
This can certainly be an argument to try to improve build time later on.

maybe some of the .github/actions code can be removed as well?

I forgot to remove them, they are all unneeded now, so they are all removed now.

@diconico07 diconico07 force-pushed the revamp-build branch 2 times, most recently from 9fb822b to 53eadfa Compare October 27, 2023 08:54
Signed-off-by: Nicolas Belouin <[email protected]>
@diconico07
Copy link
Contributor Author

/version patch

@github-actions github-actions bot added the version/patch Patch version change is needed label Oct 27, 2023
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@diconico07 diconico07 merged commit c96e8db into project-akri:main Oct 27, 2023
3 checks passed
@diconico07 diconico07 deleted the revamp-build branch October 27, 2023 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version/patch Patch version change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency on outdated cross images
3 participants