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

cmake: explain BUILD_VERSION and KERNEL_VERSION_* #15950

Merged

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented May 6, 2019

  • Add examples for the latter.
  • Point at each other and highlight how independent they are from each
    other.
  • State their inputs and outputs in plain English.
  • Fix "git describe" error message giving the wrong impression that
    everyone cares about BUILD_VERSION. Only the boot banner cares now.

Signed-off-by: Marc Herbert marc.herbert@intel.com

#
# Outputs with examples::
#
# PROJECT_VERSION 1.14.99.42
Copy link
Contributor

Choose a reason for hiding this comment

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

If the idea was "to explain everything", then I'd like to point that the list below doesn't explain what is "42" at the end.

Copy link
Collaborator Author

@marc-hb marc-hb May 7, 2019

Choose a reason for hiding this comment

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

Good catch thank you, will put the PATCHLEVEL example below back in sync.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Despite the "42" example (changed, thx) the goal is to explain but not "everything". This is more about not having to read the whole cmake file or git grep the entire source code to filter out the many temporary variables and find the names that matters. The added comment points at the VERSION file where TWEAK is defined so this is hopefully enough. I considered mentioning TWEAK here but @SebastianBoe just helped me trim this down significantly (thx) so I'm reluctant to add stuff again.

cmake/git.cmake Outdated
# everything fails and leave BUILD_VERSION undefined.
#
# Inputs:
# ``BUILD_VERSION`` or the output of ``git describe``
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is

the output of git describe

an input to git.cmake ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because git.cmake execute_process( git describe) and reads its output? I don't understand the question.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The output of git describe is not an input to git.cmake, it is an internal process of git.cmake.

If we had run git describe before git.cmake, then you could say it was an input.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see why who runs git describe and when matters. The terms "input" and "output" are standard for internal calls too:
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/out-parameter-modifier
https://docs.swift.org/swift-book/LanguageGuide/Functions.html
https://www.learncpp.com/cpp-tutorial/73-passing-arguments-by-reference/
etc.

In fact CMake's documentation for execute_process() uses the terms "input" and "output" too:
https://cmake.org/cmake/help/v3.14/command/execute_process.html?highlight=output

Copy link
Collaborator

Choose a reason for hiding this comment

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

I find the documentation causes more confusion than than it clarifies, but perhaps this is subjective.

Perhaps "the output of 'git describe'" will clarify things for some users.

This is OK.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wrote the "inputs" and "outputs" section in git.cmake for consistency with version.cmake but now I see they've become mostly redundant with the plain English sentence just before. I'll drop them; less is more. I assume the first sentence is OK.
The last sentence with the reference to the (independent!) KERNEL_VERSION_* is the most important IMHO

cmake/version.cmake Outdated Show resolved Hide resolved
Copy link
Collaborator

@SebastianBoe SebastianBoe left a comment

Choose a reason for hiding this comment

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

Requesting some changes

- Add examples for the latter.
- Point at each other and highlight how independent they are from each
  other.
- State their inputs and outputs in plain English.
- Fix "git describe" error message giving the wrong impression that
  everyone cares about BUILD_VERSION. Only the boot banner cares now.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb force-pushed the cmake-explain-versions branch from 4ba10bf to b2a199b Compare May 8, 2019 22:04
@marc-hb marc-hb requested a review from SebastianBoe May 8, 2019 22:20
Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

Nice addition, thanks!

@carlescufi carlescufi merged commit a880fb1 into zephyrproject-rtos:master May 9, 2019
@marc-hb marc-hb deleted the cmake-explain-versions branch May 30, 2019 23:25
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.

4 participants