-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add VISS code when building with defaults #19
Add VISS code when building with defaults #19
Conversation
Including this feature in releases seems fine to me. But do we really want to add it as a default feature? It doesn't seem like something a majority of users want enabled. I think it makes more sense build releases with this feature (manually) enabled. |
I do not think that the every day user wants this feature. |
Converted to draft. More work than I thought, I modified the build script, but I noticed the GH action build has been changed to not even use it. Before changing back might want to have a discussion why that happened. I know the reasons why this was shell-scripted before
One nice thing I saw in the change is the use of the"matrix" thingy (https://github.com/eclipse-kuksa/kuksa-databroker/blame/main/.github/workflows/kuksa_databroker_build.yml) which is nifty. Are there other reasons for "rewriting" the build script? Should this be combined? I.e. with the changes done for the SBOM generation not yet ported from the old repo, the bash script might likely also be called "matrix-style" from GH actions. (the current version not, as it just linearzy build all supported targets). Would that be a worthwhile way forward? In general I don"t think we should do "the same thing" twice @lukasmittag as for the why: if you live in KUKSA world alone, you likely would not want to touch it. But in the larger VSS ecosystem VSS often comes up as "one standard" or "lowest common denominator", so it would be good to have it "easily" available out of the box, so that "activation" works without recompiling. It would for example enable integration into the COVESA data services playground https://github.com/COVESA/cdsp |
Building all the target platforms in parallel was the reason for not using the build script. Time to compile for all target platforms:
The difference is mostly because of building in parallel, but also because the script is generating the BOM and hence needs to install the requirements for that before it can start building. That is better done in a separate build step as the BOM is not needed until packaging the (release) artifacts.
Maybe. Compiling for certain platform just uses cargo / cross, so there's not that much need for a script, but I guess it doesn't hurt to have that functionality in a script as well. A script would mostly make sense for consistently generating the BOM / packaging the artifacts I guess. Bottom line, as a developer I vastly prefer fast build times / feedback over any theoretical portability to a different build system in the future. But being able to easily run the CI build steps locally is definitely beneficial. |
Changing the github action is literally a one-liner if we want to enable VISS in releases now. diff --git a/.github/workflows/kuksa_databroker_build.yml b/.github/workflows/kuksa_databroker_build.yml
index cdb9bdd..d1e8c56 100644
--- a/.github/workflows/kuksa_databroker_build.yml
+++ b/.github/workflows/kuksa_databroker_build.yml
@@ -129,7 +129,7 @@ jobs:
- name: Build
working-directory: ${{github.workspace}}/
run: |
- cross build --target ${{ matrix.platform.target }} --bin databroker --release
+ cross build --target ${{ matrix.platform.target }} --features viss --bin databroker --release
mkdir -p "dist"
cp "target/${{ matrix.platform.target }}/release/databroker" "dist"
- name: Package dist files |
a39cd40
to
7319210
Compare
Thanks. Did not expect the time savings where such significant. Also, I can't defuse the "it is (less than) a one-line change" argument So let's not overcomplicate this PR. I will look into this, when putting the sbom changes in another PR if there is some way tor reconcile advantages/reducing duplication. For now I think this should be good to merge. Approve if you agree :) |
7319210
to
1db36d9
Compare
1db36d9
to
145251d
Compare
@@ -18,6 +18,15 @@ SCRIPT_DIR=$(dirname "$SCRIPT_PATH") | |||
|
|||
CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse | |||
|
|||
# Check if a certai feature set was requested | |||
if [ -z "$KUKSA_DATABROKER_FEATURES" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should better document (in our wiki? As part of release testing? ) how to set this variable when building locally to get viss support. The only use-case I know of right now for this shell script is if I want to build docker containers locally, as our Dockerfile relies on that binaries already exist.
Might spend some time trying this out unless you have already tested thoroughly Sebastian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it, but as seen above, this is not currently used, as the build is done in GH Actions. For now I did the "easy fix" as suggested by John, but I want to give the capability to select features to the build script as well.
I might reconcile the two ways of building in a follow up PR (the build script will change again), so I think documenting it should be done then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been used by me :-)
In my view it is beneficial to have one way to build docker containers locally, good if doing major changes
…build script Signed-off-by: Sebastian Schildt <[email protected]>
145251d
to
c8af36a
Compare
We discussed before, that our released/default builds should include the experimental VISS code.
As this is still hidden behind a (default-off) runtime flag it should not incurr any safety, stability or performance issues during runtime, unless enabled