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.

Sign up for GitHub

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

Jump to bottom

cmake: Set kernel version to NCS version #11956

Merged
merged 3 commits into from
Sep 20, 2023
Merged

cmake: Set kernel version to NCS version #11956

merged 3 commits into from
Sep 20, 2023

Conversation

nordicjm
Copy link
Contributor

@nordicjm nordicjm commented Aug 4, 2023 

Sets the zephyr kernel version to the version of NCS so that the
boot banner displays the correct NCS details instead of sdk-zephyr
details

and

manifest: Update sdk-zephyr revision

Adds the boot banner override features which displays the NCS
version

@github-actions github-actions bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. manifest labels Aug 4, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Aug 4, 2023 •
edited
Loading

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
zephyr nrfconnect/sdk-zephyr@c709414 nrfconnect/sdk-zephyr@96a0d4e (main) nrfconnect/sdk-zephyr@c7094146..96a0d4e8

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@nordicjm nordicjm mentioned this pull request Aug 4, 2023
Merged
@nordicjm nordicjm requested a review from krish2718 August 4, 2023 08:59
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Aug 4, 2023 •
edited
Loading

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
desktop52_verification X
test-ci-nrfconnect-boot-fw-update X
test-fw-nrfconnect-nrf_crypto X
test-fw-nrfconnect-tfm X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

@krish2718 krish2718 mentioned this pull request Aug 4, 2023
Closed
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

tejlmand
tejlmand requested changes Aug 4, 2023
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

please ensure we don't introduce zephyrproject-rtos/zephyr#39503 in our downstream solution

cmake/kernel_version.cmake Outdated
# Set version of nRF connect SDK to BUILD_VERSION for boot banner usage
find_package(Git QUIET)
if(GIT_FOUND)
execute_process(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is generally unsafe as an update / commit to a C file will not cause a CMake rerun and hence the git describe commit may be out-of-sync with the actual commit used for the build wrt. incremental builds.

See more details here for a similar (now fixed) flaw upstream: zephyrproject-rtos/zephyr#39503

and a proper fix here: zephyrproject-rtos/zephyr#42527

Notice how the upstream solution uses an add_custom_command() to drive the CMake script which does the git describe call, whereby the git command can have a proper dependency on the git index file.

@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Aug 18, 2023
@nordicjm nordicjm requested a review from tejlmand August 18, 2023 12:36
@github-actions github-actions bot removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Aug 18, 2023
wiba-nordic
wiba-nordic approved these changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@wiba-nordic wiba-nordic left a comment

Choose a reason for hiding this comment

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

Docs look good.

tejlmand
tejlmand requested changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

temp blocking.

tejlmand
tejlmand approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

lgtm

@NordicBuilder NordicBuilder removed the DNM label Sep 7, 2023
tejlmand and others added 3 commits September 18, 2023 10:40
@tejlmand @nordicjm
cmake: create ncs_version.h through the Zephyr version infrastructure
1f0884a 
This commit changes the generation of ncs_version.h from internal
configure_file() call to use the Zephyr provided mechanism.

This is first step in NCS adopting the uniform VERSION approach provided
by Zephyr.

Furthermore it injects ncs_version.h into Zephyr's version.h allowing
NCS to substitute the build version in the boot banner, unless build
version has been defined by the user as `-DBUILD_VERSION=<version>`.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@nordicjm
Kconfig: Override default boot banner
92d99cf 
Overrides the default boot banner with NCS

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm
doc: release notes: Add note on NCS boot banner
39ec4b4 
Adds a note that the default boot banner now shows as nRF
Connect SDK

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@github-actions github-actions bot removed the manifest label Sep 18, 2023
@nordicjm nordicjm merged commit ea3767a into nrfconnect:main Sep 20, 2023
16 checks passed
@nordicjm nordicjm deleted the kernelversion branch September 25, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tejlmand tejlmand tejlmand approved these changes

@wiba-nordic wiba-nordic wiba-nordic approved these changes

@krish2718 krish2718 krish2718 approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees
No one assigned
Labels
doc-required PR must not be merged without tech writer approval. manifest-zephyr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nordicjm @NordicBuilder @tejlmand @wiba-nordic @krish2718