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

Remove vss-tools submodule and update CI #768

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

Conversation

SebastianSchildt
Copy link
Collaborator

@SebastianSchildt SebastianSchildt commented Aug 20, 2024

This is a first try to remove vss-tools submodule (which I do believe we should do)

It works "perfectly"... except.... binary exporter because that would need some special treatment as it can not be installed but has some component that needs to be compiled "by hand".

Advantages

Open question: Makefile

Generally I feel the makefile is a bit "anchronistic" and for testing purposes I would move the logic into GH actions. As a "user" I never ever used the Makefile (it seems an extra layer, I would expect: "This is the to tool (vss-tools) and here is how to use it)
I do now however it has (had?) fans, so I am also fine with keeping it

Options for binary

  • Ignore (as it is now)
  • Write some extra script/test to check it
  • Somehow make it "easily" available in pip install. However compiling things is not that easy for python packages and we would need to to make sure it at least runs on major platofrms such as arm, amd64, riscv, and than still a windows user might come aorund -> I don't think we should do that.
  • Rewrite it in Python (likely equally unrealistic) made an attempt: Removed C dependency from binary exporter vss-tools#395
  • Other options?

Todo

[ ] Update docs
[ ] Decide binary
[ ] Keep makefile?

pip install pytest
# idlc currently used both during python test and later in this script
pip install cyclonedds
pip install git+https://github.com/COVESA/vss-tools@master
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using master as reference could give us some problems. Like recently when we did quite large refactoring of vss-tools. If we would have referred to master like here then VSS build on master would fail as we would drag in the new incompatible CLI syntax. So far upgrading vss-tools version have been a deliberate action, and to update the submodule you also need to fix regressions, if any. So the question is - do we see it as OK that VSS builds sometimes fails because of changes in vss-tools (even if vss repo is unchanged)

With that said, I sort of like the idea to get rid of the submodule. But what we possibly could do instead would be to rely on pypi-releases, including pre-releases. Or at least do it as part of release preparations. I.e. when we prepare the VSS 5.0 Release Candidate we could instead of master use a reference to 5.0rc0 in pypi. Sometimes like that is anyway needed for maintenance/release-branches, as an older VSS version may not necessarily may be compatible with a newer vss-tools version.

@erikbosch
Copy link
Collaborator

Concerning Makefile - for me it has been a simple way to check locally that my changes in the vspec are correct, i.e. easier to just write make csv than writing the expanded command as a sanity check, and it has also worked quite well as example for showing people how they can use the tooling. But I have no strong opinion there, i can live without it as well.

@erikbosch
Copy link
Collaborator

Another topic related to Makefile - today we have a VERSION file that is used when generating release artifacts, to name output files based on VERSION number. I am not sure if we need to migrate that functionality from Makefile to build actions, the files could have the same name regardless of version.

@erikbosch
Copy link
Collaborator

MoM:

  • Removal depends on binary installation
  • Please review, does anyone have a strong preference on keeping submodule

@erikbosch
Copy link
Collaborator

MoM:

  • May need refactoring after binary PR merged
  • OK to merge after updated and verified

@erikbosch
Copy link
Collaborator

I have another PR in #772 trying to just update existing submodule and adding "new tools". Maybe we should merge that one first and then rebase this one upon it.

Concerning Makefile - that it is a Makefile does not really matter to me, but I think it is good to have a short hand command to generate/recreate output for local branch. Just having the vspec export ... hidden in a build action is maybe not optimal if you want to test locally. An alternative approach could be to have a script folder with scripts like scripts/build_binary.sh, scripts/build_csv.sh and so on that could be used both from Github CI and locally.

Now when we are getting more tools I am also thinking if we should create something like https://github.com/eclipse-kuksa/kuksa-databroker/blob/main/.github/workflows/create_draft_release.yml i.e. a manually triggered workflow that create and upload all artifacts that we want to include. But that could be handled in a separate PR, i.e. let this one focus on submodule removal.

@erikbosch erikbosch force-pushed the remove_submodule branch 2 times, most recently from 0531e7a to 86b2953 Compare September 16, 2024 13:22
@erikbosch erikbosch marked this pull request as ready for review September 16, 2024 13:23
@erikbosch
Copy link
Collaborator

I did some more changes which I think would make sense to introduce for the upcoming 5.0, in short building on what Mr Schildt already have done.

In short - adding a release workflow that triggers build and uploads what we want to upload. Also removing the VERSION file as I do not really see that it is needed and using generic names for build artifacts making handling easier.

An example on how this could affect release handling.

  • If we want to release a v1.6rc0 from master then we trigger the Draft release workflow stating 1.6.rc0 as version.
  • That will build all artifacts and upload them to a draft release
  • When publishing the release source code is added and tagging performed and we get something like https://github.com/erikbosch/vehicle_signal_specification/releases/tag/v1.6rc0
  • (In a real release we should update release notes before publishing)

FYI @SebastianSchildt @sschleemilch

@erikbosch erikbosch added the Release Blocker PR suggested to be merged before releasing label Sep 17, 2024
@@ -20,11 +28,10 @@ jobs:
python -V
python -m pip --quiet --no-input install --upgrade pip
python -m pip --quiet --no-input install --upgrade wheel
cd vss-tools
pip install -e .
pip install pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need pytest anymore, probably the same for wheel?

@SebastianSchildt SebastianSchildt force-pushed the remove_submodule branch 5 times, most recently from 7b2325a to 2d78e44 Compare September 17, 2024 12:50
Signed-off-by: Sebastian Schildt <[email protected]>
@SebastianSchildt
Copy link
Collaborator Author

This is an example how we can do a matrix configuration testing different versions. Still draft, because the used tags are not correct, to show the behavior in term sof problems.

This only supports "new-style" tools. I don't think we should invest the effort to write different tests for old style (<= v4) tools.

Also this still uses the Makefile, to keep changes at minimum.

Current matrix

buildtest:

    continue-on-error: ${{ matrix.experimental }}
    strategy:
      matrix: # this needs to be an array. We should only list "known good/expected to work" vss-tools versions here
        vss-tools-version: [master]
        experimental: [false]
        include: # this need to be single entities, this should list master and poentially other experimental versions
          - vss-tools-version: v4
            experimental: true

So for a "real" case the virst section, with experimental false should contain a tag for v5 tools (or an rc/pre tag). Not a moving target like master. Currently it seems no newstyle tools tagged in vss-tools so I used master for demo

in the experimental=true section we can add master (currently I used v4, to have an exmaple of something that defintely fails)

The continue-on-error: ${{ matrix.experimental }} in the job makes sure, that the workflow continues even if experimental (v4 in this case) fails. It does show a failed check, so maintainers need to check whether that is acceptable before merge.

An alternative are the (currently commented) continue-on-error: ${{ matrix.experimental }}in each of the job steps, In that case a fail in an experimental branch would still list as "all green" (e.g. would be 6 sucessful checks in this example). I think that is not so good, because failures are not very transaprent then.

@erikbosch erikbosch removed the Release Blocker PR suggested to be merged before releasing label Sep 17, 2024
continue-on-error: ${{ matrix.experimental }}
strategy:
matrix: # this needs to be an array. We should only list "known good/expected to work" vss-tools versions here
vss-tools-version: [master]
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SebastianSchildt - I am thinking on how this possibly can be integrated with my ideas in #778

What I think we need is one vss-tools version which is the "main" version for this particular commit. I.e. the version we shall use to generate release artifacts. It could be "master" for a regular commit (not intended to be released) but for release candidates and releases it should point to either a tag (like v4, v4.0, v4.0rc0, v5.1dev0 or similar) or a pypi release. And if we want a "release workflow" like in #778 we preferably need to have a static way to address the "official" vss-tools version (like an alias), so that we easily can fetch
download from the folder "current" or similar.

You are better in github actions than I am so you most likely have some good ideas.

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.

3 participants