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

Add xrt::version APIs #7696

Merged
merged 6 commits into from
Sep 9, 2023
Merged

Add xrt::version APIs #7696

merged 6 commits into from
Sep 9, 2023

Conversation

stsoe
Copy link
Collaborator

@stsoe stsoe commented Sep 6, 2023

Problem solved by the commit

Ability to check XRT version and build at run-time.

Ability to checkout XRT version and build at run-time.

Signed-off-by: Soren Soe <[email protected]>
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 6, 2023

Build failed :(

In the GNU C Library, "major" is defined
by <sys/sysmacros.h>. For historical compatibility, it is
currently defined by <sys/types.h> as well, but we plan to
remove this soon. To use "major", include <sys/sysmacros.h>
directly. If you did not intend to use a system-defined macro
"major", you should undefine it after including <sys/types.h>.

Signed-off-by: Soren Soe <[email protected]>
@stsoe stsoe force-pushed the version branch 2 times, most recently from 256a8e5 to 99c0f6c Compare September 7, 2023 02:16
Signed-off-by: Soren Soe <[email protected]>
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 7, 2023

Build failed :(

2 similar comments
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 7, 2023

Build failed :(

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 7, 2023

Build failed :(

@stsoe
Copy link
Collaborator Author

stsoe commented Sep 7, 2023

retest this please - seems to build correctly when using build_edge -aarch versal locally

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 7, 2023

Build failed :(

1 similar comment
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 8, 2023

Build failed :(

@xilinxgitops
Copy link

retest this please - debugging issue with Vamshi

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 8, 2023

Build failed :(

@keryell
Copy link
Member

keryell commented Sep 8, 2023

I am discovering this sys/sysmacros.h nightmare. :-(
Is sys/types.h required by the XRT API? Perhaps this API enabling file-descriptor sharing?
Could this plague contained in only the internal .c/.cpp files using it?

@stsoe
Copy link
Collaborator Author

stsoe commented Sep 8, 2023

Is sys/types.h required by the XRT API? Perhaps this API enabling file-descriptor sharing?
Could this plague contained in only the internal .c/.cpp files using it?

I admit I didn't track down where this include came from, but xrt_version.cpp does not include (implicitly) anything but <string> and <iostream> .

I suspect iostream is the culprit, and yes, I don't really fancy iostream being included especially not in a header file, but that's unfortunately how the version.h file generated by CMake was coded up. Let me revisit that, and cleanup src/CMake/config/version.h.in and move the definition of the print function into the xrt_version.cpp. While a proper change, this doesn't actually solve the major minor macro mess.

@manikandan-xilinx
Copy link
Collaborator

retest this please. (Including hidden files during checkout)

@keryell
Copy link
Member

keryell commented Sep 8, 2023

Curious.
I cannot imagine C++ <iostream> or anything else pulling anything from sys/ defining some atrocious macros like that.

@stsoe
Copy link
Collaborator Author

stsoe commented Sep 8, 2023

Well I don't know, but as I said, the macro mess is isolated to xrt_version.cpp which includes gen/version.h which in turn includes <string> and <iostream>

Btw., the error was isolated to edge builds for aarch64, not x64

The version.h is generated at CMake time from
XRT/src/CMake/config/version.h.in by XRT/src/CMake/version.cmake.

version.cmake uses ${GIT_EXECUTABLE} to get meta data specific to
version of XRT being built, but none of the data is gathered in CI
when running `build_edge.sh -aarch versal -full` and the version.h
file is missing Git data which instead ends up as empty string.

XRT_BUILD is a define macro based on number of commits acquired from
Git, but if this value is empty the use of the macro fails.  This
PR defaults the value to 0 in hopes it gets overwritten by Git.

Signed-off-by: Soren Soe <[email protected]>
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 8, 2023

Build failed :(

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 8, 2023

Build failed :(

1 similar comment
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 8, 2023

Build failed :(

The feature id (xrt::version::feature()) is defined as the total
number of commits to XRT main branch.  For branches off of XRT's main
branch, the feature number is the total number of commits at the time
the branch diverged from XRT's main branch.

The feature id is computed as the difference between number of commits
on current branch and the number of commits since origin of branch from
master.

(git rev-list HEAD --count) - (git rev-list HEAD ^origin/master --count)

Signed-off-by: Soren Soe <[email protected]>
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 9, 2023

Build Passed!

@stsoe stsoe merged commit c885145 into Xilinx:master Sep 9, 2023
2 checks passed
@stsoe stsoe deleted the version branch September 9, 2023 23:19
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.

5 participants