pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.12k stars 688 forks source link

build(cmake): fix configure_file version header #1131

Closed shlomnissan closed 1 year ago

shlomnissan commented 1 year ago

A relative path for configure_file <output> is treated with respect to the value of CMAKE_CURRENT_BINARY_DIR. This PR adds an absolute path to the output parameter so version.h is generated and stored in the include directory.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: +0.15 :tada:

Comparison is base (a68ad09) 78.48% compared to head (c7d67d0) 78.64%.

:exclamation: Current head c7d67d0 differs from pull request most recent head 7f4f03a. Consider uploading reports for the commit 7f4f03a to get more accurate results

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1131 +/- ## ========================================== + Coverage 78.48% 78.64% +0.15% ========================================== Files 53 53 Lines 6892 6892 ========================================== + Hits 5409 5420 +11 + Misses 1483 1472 -11 ``` [see 3 files with indirect coverage changes](https://app.codecov.io/gh/pistacheio/pistache/pull/1131/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

kiplingw commented 1 year ago

Thanks @shlomnissan. Your PR looks mostly fine to me, but I have two comments.

(1) I am curious why include/pistache/version.h has been added to your PR since it's suppose to be machine generated at configure time from the include/pistache/version.h.in template. Perhaps this was by accident?

(2) Does the same issue you are trying to address also affect the Meson build environment?

@Tachi107, any feedback?

Tachi107 commented 1 year ago

Hi @shlomnissan, thanks for your patch. I believe that the current behaviour is the intended one, i.e. the generated file should reside in the a build directory that gets added to the include list by CMake or Meson. Why do you think that the version file should be generated in the source directory?

kiplingw commented 1 year ago

I actually didn't realize that he was trying to output it into the source directory. But yes, @Tachi107 is correct. That's how build environments normally are suppose to work. Since the source directory in some situations might be read only, that's the only way to allow remote builds.

shlomnissan commented 1 year ago

Interesting... I didn't know that the compiler driver includes the build folder in its header search paths.

Suppose I want to use the version variables defined in version.h to update the user agent header value in the client so that it's not hard-coded (client.cc:33). How would I include it?

kiplingw commented 1 year ago

The version.h ships with the binary package with the versioning baked into it. You'd just use the variables that are in it already in your application. But note that those will only say the version it was compiled against. If the library is still binary compatible then it's possible the application could be linking at runtime with a version newer than what it saw at compile time.

shlomnissan commented 1 year ago

@kiplingw Thank you for the feedback, and I apologize for the delayed response. I’m going to close the PR. However, I am still uncertain about how to use version.h in my application with my existing workflow. Specifically, I am including the source code in my project and adding it with CMake's add_subdirectory. The current binary directory is not part of the default header search path, so my project can’t find it.

kiplingw commented 1 year ago

Hey @shlomnissan,

I strongly recommend not including the project source within your project. If you want to make source level changes, then by all means do that. But my guess is you probably just want to use the library.

If the latter, at configure / build time you want to use pkg-config(1), as discussed here. The pkg-config manifest ships with the binary packages that you can find here.

As for using version.h, you're going to do something like this:

    #include <pistache/version.h>

    cout << Pistache::Version::Major << "." << Pistache::Version::Minor << "." << Pistache::Version::Patch << ".-git" << Pistache::Version::Git);

Hopefully that's helpful.