-
Notifications
You must be signed in to change notification settings - Fork 168
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
base: master
Are you sure you want to change the base?
Conversation
.github/workflows/buildcheck.yml
Outdated
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 |
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.
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.
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 |
Another topic related to |
MoM:
|
MoM:
|
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 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. |
0531e7a
to
86b2953
Compare
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 An example on how this could affect release handling.
|
Signed-off-by: Sebastian Schildt <[email protected]>
86b2953
to
2e52169
Compare
f93e200
to
41dbd74
Compare
@@ -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 |
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 don't think we need pytest anymore, probably the same for wheel?
7b2325a
to
2d78e44
Compare
Signed-off-by: Sebastian Schildt <[email protected]>
2d78e44
to
13f2ccf
Compare
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 An alternative are the (currently commented) |
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] |
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.
@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.
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
(likely equally unrealistic)made an attempt: Removed C dependency from binary exporter vss-tools#395Todo
[ ] Update docs
[ ] Decide binary
[ ] Keep makefile?