🔨 (cmake): FirmwareKit - depend on os_version file #1231
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #1231 +/- ##
========================================
Coverage 96.16% 96.16%
========================================
Files 146 146
Lines 3594 3594
========================================
Hits 3456 3456
Misses 138 138 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
ladislas
force-pushed
the
ladislas/feature/cmake-depend-os_version
branch
2 times, most recently
from
3c4afb2 to
2b9f7b1
Compare
YannLocatelli
requested changes
Jan 12, 2023
Comment on lines
+30
to
+34
| Version current_version = Version { | ||
| semver::version {OS_VERSION}.major, | ||
| semver::version {OS_VERSION}.minor, | ||
| semver::version {OS_VERSION}.patch, | ||
| }; |
There was a problem hiding this comment.
This is in conflict with issue
There was a problem hiding this comment.
yes and no -- the idea of removing semver was to remove complicated code just to get the version number.
semver is using constexpr almost everywhere, which means that the actual work is done at compile time and not run time, so the final binary should only have the actual major, minor and patch values.
this must be further investigated to be confirmed.
on the other hand, removing semver will mean finding another way to parse the OS_VERSION, either by using lines for each values or different files. A bit more complicated to handle cleanly in cmake.
also, semver is already being used in embedded code, so using it in unit tests is normal and cleans up things while we investigate further and decide to keep it or remove it for something else.
ladislas
force-pushed
the
ladislas/feature/cmake-depend-os_version
branch
from
2b9f7b1 to
26a4fcf
Compare
YannLocatelli
approved these changes
Jan 13, 2023
This commit make FirmwareKit (and directory) depend on os_version config file This means that when the version is updated, cmake configure is run again and the os_version.h file is regenerated with the new version This should make development as build number could be added in the future as well
ladislas
force-pushed
the
ladislas/feature/cmake-depend-os_version
branch
from
26a4fcf to
d873d33
Compare
ladislas
force-pushed
the
ladislas/feature/cmake-depend-os_version
branch
from
d873d33 to
7f77528
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit make FirmwareKit (and directory) depend on os_version config
file
This means that when the version is updated, cmake configure is run
again and the os_version.h file is regenerated with the new version
This should make development as build number could be added in the
future as well