Make BufferWriter.h, BufferWriterForward.h and MemSpan.h available to C++ plugins. #3361
Conversation
lib/ts/Makefile.am
Outdated
| library_includedir=$(includedir)/ts | ||
|
|
||
| library_include_HEADERS = apidefs.h string_view.h TextView.h | ||
| # IMPORTANT: These headers are exported for use by plugings. Any traffic server headers they in turn include, directly or |
There was a problem hiding this comment.
OK, I'll also fix another place this was misspelled that I found with fgrep/find.
| BufferWriter.h \ | ||
| BufferWriterForward.h \ | ||
| MemSpan.h \ | ||
| TextView.h |
There was a problem hiding this comment.
So this is effectively a white-list of headers that will be "publicly" visible to the plugins?
There was a problem hiding this comment.
Yes, they're copied to include/ts in the install tree.
There was a problem hiding this comment.
OK, white-list sounds good. Nice comment there too.
|
I'm working on cleaning up these headers to remove dependencies on ATS internals. On a first pass, it looks like it is mainly I think it's fine to put |
d2r
left a comment
d2r
left a comment
There was a problem hiding this comment.
Changes seem OK to me. I did not do a thorough check of all transitively included header files, but I agree with @ywkaras that many cases in which we missed something would be revealed as compile errors and we aren't seeing those.
@SolidWallOfCode do you have any other concerns?
|
One thing to consider is the issue of any sort of ABI for C++ plugins. I think, with this change, we would definitely have to require that plugins were built with the same compiler and options as the core. |
|
Example showing vague linkage does not occur with dynamically linked libraries. https://gist.github.com/ywkaras/2d28c3111903125eb47b9b594da4f905 |
|
I really dislike the |
|
In general, the header files for an interface will likely include other header files that should not be part of the interface. Maybe there's a better way to do it, but I don't think it's something that necessarily can be avoided. |
|
[approve ci clang-format] |
|
I think we could do better, if necessary by splitting files so that the exposed interface files can include files that are not inappropriate for plugins. |
There was a problem hiding this comment.
I don't thin we need to have a ts_no_api directory. Only header files that are in the include install directories should be used for plugins. Also, mixing C++ and C files in the C header install directory (e.g. /usr/local/include/ts/) shouldn't be done.
d2r
dismissed
their stale review
Dismissing because more discussion has revealed better approaches to this.
|
I have eliminated ts_no_api. @bryancall and @SolidWallOfCode can have a committer fight about the rest. |
|
[approve ci CentOS] |
…ink_apidefs.h available to C++ plugins.
|
[approve ci autest] |
|
Superseded by #3724. |
6 participants
No description provided.