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 find_package, enable CPack and support clang-cl #338

Merged
merged 10 commits into from
Nov 2, 2023

Conversation

ToKiNoBug
Copy link
Contributor

@ToKiNoBug ToKiNoBug commented Oct 28, 2023

This PR aims to make kompute easier to be used as a C++ SDK.

  1. Add support to clang-cl
    Before this clang-cl is treated like g++ or clang++, and many -W flags are passed to it, but clang-cl is a MSVC-like frontend.

  2. Fix an error when kompute is being imported as a cmake package
    komputeTargets.cmake is required in a find_package call, but this file is not generated previously.

  3. Fix a bug to support fmtlib v10
    fmtlib v10 doesn't think std::array<char,N> is formattable, so we use .data().

  4. Fix KOMPUTE_OPT_INSTALL.
    Previously this option doesn't work because install calls will always take effect regardless of the value of KOMPUTE_OPT_INSTALL. Now no kompute files will be installed if KOMPUTE_OPT_INSTALL is set to OFF.

  5. Enable CPack to generate many kinds of software packages like deb, rpm, zip, 7z, mac bundle, NSIS, etc.
    Configurations for many other kinds of packages can be added in future.
    To use cpack, run cpack -G <generator> in the build directory. For example:

    cd build
    cpack -G TXZ
    cpack -G DEB

    This will generate a deb package and a tar.xz package.

Signed-off-by: ToKiNoBug <tokinobug@163.com>
Signed-off-by: ToKiNoBug <tokinobug@163.com>
Signed-off-by: ToKiNoBug <tokinobug@163.com>
Signed-off-by: ToKiNoBug <tokinobug@163.com>
Signed-off-by: ToKiNoBug <tokinobug@163.com>
Signed-off-by: ToKiNoBug <tokinobug@163.com>
@ToKiNoBug
Copy link
Contributor Author

ToKiNoBug commented Oct 28, 2023

Oh, I see the python tests failed. I'm fixing it.

Python build fixed.

@ToKiNoBug
Copy link
Contributor Author

Besides, I found that gcc13 can't build kompute because built in fmtlib(v8.1.1) has a possibly dangling reference to a temporary warning, and this is turned to an error by extra -W compiler flags.
Either removing extra -W options, or updating fmtlib to v10.0.0 will fix this problem. I don't know which plan is better.

Signed-off-by: ToKiNoBug <tokinobug@163.com>
@axsaucedo
Copy link
Member

Either removing extra -W options, or updating fmtlib to v10.0.0 will fix this problem. I don't know which plan is better.
Do you have more info on this? I also don't have an huge issue with upgrading fmtlib but we woudl also have to test that it works with the rest of the dependencies (ie spdlog), as well as if there are any other issues / changes required to the codebase.

I would be keen to continue with strict -W options as it ensures a clean / maintainable codebase even if often can be a pain intially when introducing

@ToKiNoBug
Copy link
Contributor Author

I would be keen to continue with strict -W options as it ensures a clean / maintainable codebase even if often can be a pain intially when introducing

We can update the version of builtin fmtlib to v10.1.1.

Althoug we can't control the version of external fmtlib, we can report a warning/error if users are building kompute with gcc13 or later with fmtlib v9 or earlier.

@axsaucedo
Copy link
Member

We can update the version of builtin fmtlib to v10.1.1.
Yes this seems reasonable.

Althoug we can't control the version of external fmtlib, we can report a warning/error if users are building kompute with gcc13 or later with fmtlib v9 or earlier.
Absolutely, if we can do a warning without too much complexity on our side that could be helpful, but agree that we can't control what others bring in - we can make it clear that to minimise warnings requires this version - similarly that we don't run tests with other versions

Signed-off-by: ToKiNoBug <tokinobug@163.com>
@ToKiNoBug
Copy link
Contributor Author

Absolutely, if we can do a warning without too much complexity on our side that could be helpful, but agree that we can't control what others bring in - we can make it clear that to minimise warnings requires this version - similarly that we don't run tests with other versions

I have updated the version of builtin fmt to 10.1.1. But I found it hard to provide a warning about old fmt without complexity. Older fmt doesn't tell its version, may be there's some tricks to get its version, but the code will be too complex.

Signed-off-by: ToKiNoBug <tokinobug@163.com>
Signed-off-by: ToKiNoBug <tokinobug@163.com>
@ToKiNoBug
Copy link
Contributor Author

I hope to know if there is anything I should do about this PR. Any problem or requirement?

Copy link
Member

@axsaucedo axsaucedo left a comment

Choose a reason for hiding this comment

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

Looks good - thank you for the contribution

@axsaucedo axsaucedo merged commit df0ab79 into KomputeProject:master Nov 2, 2023
8 checks passed
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.

2 participants