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
WebTorrent support #4123
WebTorrent support #4123
Changes from all commits
679f037
99ce898
144688c
543e761
3bb0950
1074feb
105b35f
6db6634
2e066c3
022fca2
fff9d92
2f9e167
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.
what's the rationale for this change? I'm no expert on CMake, but it seems like a step backwards to use global variables rather than associating build properties with targets
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 changed it because
add_compile_options
adds options also for C code, which breaks the compilation of libdatachannel's dependencies. Since the options were added globally before anyway I changed toCMAKE_CXX_FLAGS
to make them exclusive to C++.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.
oh, I see. I didn't realize at first that this adds global compiler flags. I'm a bit surprised they aren't just added to the
libtorrent
target.@zeule do you have any thoughts on this?
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.
No, I don't know an elegant way to achieve what we want, because we want those flags to be defined for the libtorrent target and the tests, but we can't make them public libtorrent flags because we don't want them to escape into the libtorrent interface (via pkg-config or cmake targets).
These two solutions seem to be least ugly to me:
$<BUILD_INTERFACE:
generator expression, so that they will not escape to the exported target.The second one needs testing for the pkg-config generator, but otherwise is, probably, the best way to go.
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.
option two seems risky to me. it would mean anyone adding libtorrent as a cmake dependency would get all these flags too, right?
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.
Yes, if they reference the target from a build tree.