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

build(cmake): fix configure_file version header #1131

Closed

Conversation

shlomnissan
Copy link
Contributor

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
Copy link

codecov-commenter commented May 21, 2023

Codecov Report

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

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

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

❗ 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
@@            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

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

@kiplingw
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

shlomnissan commented May 27, 2023

@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
Copy link
Member

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.

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.

None yet

4 participants