Skip to content

Fix cpp header issues - #2733

Merged
Jens-G merged 2 commits into
apache:masterfrom
cfriedt:fix-header-issues
Nov 22, 2022
Merged

Jens-G merged 2 commits into
apache:masterfrom
cfriedt:fix-header-issues

Conversation

@cfriedt

@cfriedt cfriedt commented Nov 22, 2022

Copy link
Copy Markdown
Contributor

2 commits to address 2 issues:

cc @Jens-G

Chris Friedt added 2 commits November 22, 2022 05:57
The `<thrift/numeric_cast.h>` header should be part of the
installed thrift artifacts since it is included by other
installed headers.

Signed-off-by: Chris Friedt <cfriedt@meta.com>
The `PlatformSocket.h` defines some macros that reference
symbols in the libc without including the necessary headers.

Moreover, the headers are protected by config macros
(i.e. `#ifdef HAVE_UNISTD_H`). This makes header ordering
rather fragile.

Explicitly include `<thrift/config.h>` and conditionally
include `<unistd.h>` prior to referencing them.

A future improvement to reduce fragility would be to add
includes directly in `PlatformSocket.h`.

Signed-off-by: Chris Friedt <cfriedt@meta.com>
@Jens-G
Jens-G merged commit 850c61f into apache:master Nov 22, 2022
@Jens-G

Jens-G commented Nov 22, 2022

Copy link
Copy Markdown
Member

I merged it because it changes the situation to the better. However I would say there is sth else:
https://app.travis-ci.com/github/apache/thrift/jobs/589165720
https://app.travis-ci.com/github/apache/thrift/jobs/589165723

The c_glib tests use a C++ server to test against. Seems there are some adjustments needed too, because now it complains about missing sleep(), fork() and throw()

@Jens-G

Jens-G commented Nov 24, 2022

Copy link
Copy Markdown
Member

@cfriedt : Thoughts?

@cfriedt

cfriedt commented Nov 24, 2022 •

Copy link
Copy Markdown
Contributor Author

I merged it because it changes the situation to the better. However I would say there is sth else:
https://app.travis-ci.com/github/apache/thrift/jobs/589165720
https://app.travis-ci.com/github/apache/thrift/jobs/589165723

The c_glib tests use a C++ server to test against. Seems there are some adjustments needed too, because now it complains about missing sleep(), fork() and throw()

@Jens-G - I noticed that as well. Shall I put together a fix for c_glib? I'm not sure what the root cause is yet.

(Sorry for the late response, i had left my comment in draft by accident)

@cfriedt
cfriedt deleted the fix-header-issues branch November 24, 2022 12:35
@Jens-G

Jens-G commented Nov 24, 2022

Copy link
Copy Markdown
Member

That would be super awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants