search

IRNAS / irnas-zephyr-template

Template for Zephyr Projects
3 stars 1 forks source link

feat(version-file): add version file support #31

Closed TjazVracko closed 2 weeks ago

TjazVracko commented 4 weeks ago

Description

This commit removes the use of the version-info lib and replaces it with a python script and Zephyr's VERSION file system.

The python script version.py can be used to generate a valid VERSION file based on the provided tag or based on the output of git describe.

The app firmware no linger uses the version-info lib and instead reads the version from app_version.h, ncs_version.h, and other configs.

Closes #29

Areas of interest for the reviewer

All.

I have added the script to generate VERSION file and updated the main app. We still require updates to the CI workflows as discussed in #29. @MarkoSagadin, I am handing it off to you for the CI updates.

I am also unsure if the .gitigore and the makefile are correct. That is based on how the CI will handle the VERSION files. Keep in mind that if there are 2 apps, 2 version files have to be generated, one at the root of each "zephyr project".

Another note: The script now silently fails and sets the version to "0.0.0-0" (as described by the help text). I am not sure if we want this. Maybe it should fail if the tag is invalid. Thoughts?

Checklist

After-review steps

TjazVracko commented 4 weeks ago

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @TjazVracko and the rest of your teammates on Graphite Graphite

MarkoSagadin commented 3 weeks ago

@TjazVracko

I went through the PR and I have some thoughts. I know that we had some discussion IRL as well as in issue #29, but it think that not everything is written down.

Keep in mind that if there are 2 apps, 2 version files have to be generated, one at the root of each "zephyr project".

That is ok, but please mention this next to the gen-version target, see codechecker targets for inspiration.

I am also unsure if the .gitigore and the makefile are correct. That is based on how the CI will handle the VERSION files.

I know that I was talking in the linked issue about adding an environment variable for the version (and then ignored you when you wanted to know the name, sorry for that :sweat_smile: ), but seeing this I have a slightly different idea now.

What if:

I think that with this setup:

Does this makes sense? Am I missing some corner case?

Another note: The script now silently fails and sets the version to "0.0.0-0" (as described by the help text). I am not sure if we want this. Maybe it should fail if the tag is invalid. Thoughts?

So, I thought about this. The script should probably fail, if the given tag (or tag parsed from the git describe) is not in the correct format. If I am running this in the CI I want to know about the possible configuration mistake as soon as possible, no when then devices are in the production and I am somewhat depending on the version numbers.

MarkoSagadin commented 3 weeks ago

@MarkoSagadin I changed some things based on your suggestions:

Thank you!

My only concern is that, when someone uses the template to create a project with 2 apps, he will copy and paste the app dir, and when committing it, will not "force add" the ignored VERSION file.

That also crossed my mind, it is a reasonable concern. I would mitigate this by adding a entry to the main checklist, that link to the scripts/version/README.md which explains the use of VERSION file and the multiple app setup. (I can do this)

I would still like it if the version file was updated and committed by CI.

Can you explain why? I think that if this is done, then what we gain is only the removal of the VERSION file from the .gitignore, which would then avoid the above mentioned "force add" situation, is that right?

I am against this change as currently the release part of the CI is really only concerned with updating the CHANGELOG entry, which is completely project agnostic, and all makefile related targets are used in other workflows. I would like to stay it that way, it was designed with this idea in mind.

There is also another possible situation:

make gen-version still works in its current form, as well as the CI. But I would guess that MCUboot might complain, or there might be some cases during the development where this might not be desirable?

What do you think?

TjazVracko commented 3 weeks ago

I am against this change as currently the release part of the CI is really only c...

There are 2 problems with the committed VERSION file that is also ignored:

FIRST:

  1. VERSION file is ignored and added to the template
  2. I am developing something that needs VERSION changes, so I run the script locally to change the file.
  3. running git status now shows the VERSION file as changed
  4. running git add app/ stages the VERSION file - I might accidentally commit it.

This is annoying...., but probably won't come up often.

SECOND: this also means that the git state will be dirty when CI runs the script before generating artefacts with east release. This is not ideal, since they will be named wrong.

This means that not adding a VERSION file and ignoring it is better. But the problem now is that the build fails if no version file exists (app_version.h is not generated). This can be fixed by running make gen-version locally once.

So I think the "correct" solution is actually:

  1. add VERSION to .gitignore
  2. do not commit it to the template
  3. Modify the version script so it can generate a "dev" VERSION file, even if no git tag exists (since currently make gen-version fails if there is no release tag, which happens when a project is new)
  4. have a make command that runs the dev option described above
  5. add the call to that make command in the repo setup instructions.

We should discuss this in person a bit more, It requires a demonstration.

TjazVracko commented 3 weeks ago

@MarkoSagadin You can also modify common.conf and main.c (remove most things) and build for native_posix so you can try it out yourself.

Patch: template_for_native_posix.patch.txt

TjazVracko commented 3 weeks ago

@MarkoSagadin

Git seeing the changes to the VERISON file can be avoided using git update-index --assume-unchanged, but this would have to be run in CI and on every developer machine, so this is also not ideal.