Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Initial find script #14
base: main
Are you sure you want to change the base?
Initial find script #14
Changes from 2 commits
2cf589d
34ee6c6
e95725c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear - do we need to add
ASIO_FOUND
to theREQUIRED_VARS
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it supposed to do it by itself? Only a test can make sure of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, it would be nicer if the CMake docs would explain this point. I'll do a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, the version is required. Since this works since
asio 0.3.8
, I'd just recommend making it always part of the interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What work since asio 0.3.8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing the
asio/version.hpp
for version info using the same regex.Since 0.3.8, the version format has been the same and the regex is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to understand what to do with the asio macros:
https://think-async.com/Asio/asio-1.28.0/doc/asio/using.html#asio.using.macros
Should these all be exposed as CMake
OPTION
s, then set here intarget_compile_definitions
and a call totarget_compile_options
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to answer here. It was easy when cmake when creating the asio target. I guess when asio is system installed this is the header only version no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On ubuntu, I get the
asio.cpp
file but not theasio_ssl.cpp
.From what I understand, which is used for the pre-compiled header, however 99% of asio is implemented in header files anyways. The find script here ideally should be able to support using asio in either mode. I have only gotten the header-only version working, pre-compiled has errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess action with matrix is the way to test everything