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

fix(cmake): fixed git dev versioning. #946

Merged
merged 4 commits into from
Mar 8, 2023
Merged

fix(cmake): fixed git dev versioning. #946

merged 4 commits into from
Mar 8, 2023

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Mar 7, 2023

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area build

Does this PR require a change in the driver versions?

What this PR does / why we need it:

Fixes libs dev versioning by applying similar fixes as falcosecurity/falco#2292.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
endif()
string(REGEX MATCH "^[0-9]+.[0-9]+.[0-9]+$" libs_tag ${dev_version})
string(REGEX MATCH "^[0-9]+.[0-9]+.[0-9]+\\+driver$" driver_tag ${dev_version})
message("Tags: ${libs_tag} ${driver_tag}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example output, tested with any combination of tags:

NO TAG:
Tags:
-- Libs version: 0.10.0-rc1-475+3f5ccfe
Tags:
-- Driver version: 4.0.1+driver-419+3f5ccfe

TAG LIBS:
Tags: 0.10.5
-- Libs version: 0.10.5
Tags:
-- Driver version: 4.0.1+driver-419+3f5ccfe

TAG DRV:
Tags:
-- Libs version: 0.10.0-rc1-475+3f5ccfe
Tags:  4.0.2+driver
-- Driver version: 4.0.2+driver

BOTH:
Tags: 0.10.5
-- Libs version: 0.10.5
Tags:  4.0.2+driver
-- Driver version: 4.0.2+driver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this line that was just needed for testing ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned up!

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP changed the title fix(cmake): fixed git dev versioning. wip: fix(cmake): fixed git dev versioning. Mar 7, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 7, 2023

Wip because i spotted a bug :/

@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 7, 2023

Fixed; output is now correct in any case.

@FedeDP FedeDP changed the title wip: fix(cmake): fixed git dev versioning. fix(cmake): fixed git dev versioning. Mar 7, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 7, 2023

/milestone 0.11.0

@poiana poiana added this to the 0.11.0 milestone Mar 7, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 7, 2023

To quickly test, fetch this branch and from the HEAD of this branch, try a cmake.
Libs and driver versions should start from 0.10.4- and 4.0.1- respectively.
Then, tag libs first, eg: 0.10.5, and see that the cmake now uses correct 0.10.5 version for libs only, while driver still is 4.0.1-....
Then, tag driver, eg: 4.0.2+driver, and see that cmake picks up tagged versions for both.
Then, delete libs tag and see that cmake still picks up the 4.0.2+driver version for the driver, and a 0.10.4-... version for userspace libs.

@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 7, 2023

-- Libs version: 0.0.0
-- Driver version: 0.0.0

Ci is not picking up anything though :/

Perhaps we need to port also https://github.com/falcosecurity/falco/pull/2292/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f changes?

@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 7, 2023

Nope it was already broken (see others opened PRs). I think we just miss git client in the ci.
I will test it tomorrow.

@FedeDP FedeDP force-pushed the fix/git_versioning branch from 0fa6171 to 8ef41b8 Compare March 8, 2023 08:49
@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 8, 2023

Ok we needed to add the git safe directory since we are using a debian:buster container that runs as root.

@FedeDP FedeDP force-pushed the fix/git_versioning branch from f88f214 to bdb132f Compare March 8, 2023 09:24
@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 8, 2023

-- Libs version: 0.10.4-449+89b1a88
-- Driver version: 4.0.1+driver-445+89b1a88

Fixed!

@FedeDP FedeDP force-pushed the fix/git_versioning branch from bdb132f to 7319bf1 Compare March 8, 2023 09:31
@@ -128,7 +136,7 @@ jobs:
apt update && apt install -y --no-install-recommends ca-certificates cmake build-essential clang llvm git pkg-config autoconf automake libtool libelf-dev wget libb64-dev libc-ares-dev libcurl4-openssl-dev libssl-dev libtbb-dev libjq-dev libjsoncpp-dev libgrpc++-dev protobuf-compiler-grpc libgtest-dev libprotobuf-dev linux-headers-generic

run: |
git config --global --add safe.directory ${{ github.workspace }}
git config --global --add safe.directory $GITHUB_WORKSPACE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to use $GITHUB_WORKSPACE per this bug: actions/checkout#785

In this case we weren't affected, but since i used $GITHUB_WORKSPACE in other jobs, i decided to be consistent and use it everywhere.

@@ -87,6 +87,111 @@ function(get_git_head_revision _refspecvar _hashvar)
PARENT_SCOPE)
endfunction()

function(git_get_latest_tag _var)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is basically copy-pasted from the Falco one.

leogr
leogr previously approved these changes Mar 8, 2023
@poiana poiana added the lgtm label Mar 8, 2023
execute_process(COMMAND
"${GIT_EXECUTABLE}"
rev-list
${ARGN}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only difference from Falco one: we pass --tags too from external, because in git rev-list, --tags option takes an optional filtering argument, and we need to filter for +driver.
See https://git-scm.com/docs/git-rev-list.

function(_get_git_version _var is_driver)
# Try to obtain the exact git tag
if (is_driver)
git_get_exact_tag(tag --match=*+driver)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly like before, but we are forcing the ARGN manually now.

endif()
string(REGEX MATCH "^[0-9]+.[0-9]+.[0-9]+$" libs_tag ${dev_version})
string(REGEX MATCH "^[0-9]+.[0-9]+.[0-9]+\\+driver$" driver_tag ${dev_version})
if(dev_version MATCHES "NOTFOUND$" OR (libs_tag STREQUAL "" AND driver_tag STREQUAL ""))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Algorithm is the same as on Falco: if we are not on a tag (ie: on a x.y.z or x.y.z+driver), we fetch HEAD, then latest tag (for driver or userspace), and then we mix them together to output a diff-from-latest_tag version.

FedeDP added 2 commits March 8, 2023 10:44
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve

Copy link
Contributor

@LucaGuerra LucaGuerra left a comment

Choose a reason for hiding this comment

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

LGTM

I remember the bug in Falco that we fixed in the latest release

@poiana
Copy link
Contributor

poiana commented Mar 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, LucaGuerra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Andreagit97,FedeDP,LucaGuerra]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit 8fadd99 into master Mar 8, 2023
@poiana poiana deleted the fix/git_versioning branch March 8, 2023 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants