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

Check HAVE_CONFIG_H before including "config.h" #497

Merged
merged 1 commit into from
Jun 17, 2015

Conversation

liujisi
Copy link
Contributor

@liujisi liujisi commented Jun 13, 2015

The first step of introducing "config.h"-less BUILD.

@liujisi
Copy link
Contributor Author

liujisi commented Jun 13, 2015

@xfxyjwf for review

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Jun 13, 2015

Where is this HAVE_CONFIG_H macro defined?

@liujisi
Copy link
Contributor Author

liujisi commented Jun 13, 2015

It's defined by automake/autoconf I believe. For builds other that the automake, we need to
manually write a config.h and pass -DHAVE_CONFIG_H.

Checked the places where config.h is introduced:

  1. to generate pbconfig.h for hash maps/setes, which will be no longer needed.
  2. in common.cc for pthread related stuffs. We have macro checks for windows. Thus we need to manually pass -DHAVE_PTHREAD for bazel.
  3. gzip streams and tests, which relies on HAVE_ZLIB

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Jun 13, 2015

For the config.h, we can:

  1. Always generate config.h. This is what we currently do.
  2. Do not generate config.h. Instead of generating config.h, the build tools can just pass macros to the compiler directly (e.g., through CXXFLAGS when building with autoconf/automake).
  3. Optionally generate config.h. The code can compile both with/without config.h.

I am confused by what you are trying to do in this PR. It seems you are trying to go with option 3). But if the code is required to compile without config.h anyway, why don't we just get rid of the config.h file? (i.e., 2 is strictly better than 3 in my opinion).

@liujisi
Copy link
Contributor Author

liujisi commented Jun 13, 2015

SGTM. The only downside for option 2 is that if in the future we add need more macros from autoconf, passing -DXXX may make the output harder to read.

@liujisi
Copy link
Contributor Author

liujisi commented Jun 16, 2015

Just to capture the discussion we had yesterday.
We will remove config.h includes from srcs, rather than checking HAVE_CONFIG_H:

  • Change Makefile.am to pass in HAVR_ZLIB manually
  • HAVE_PTHREAD always needs to be set on non-windows platforms, so remove the ifdef on it.

The config.h will still be generated, because

  1. otherwise autoconf will pass those macros from -DXXXX in CXXFLAGS, which spams the logs.
  2. for users porting the code to other platforms, it is easier to see what macros are potentially needed by looking at the config.h file.

@liujisi
Copy link
Contributor Author

liujisi commented Jun 16, 2015

Removed the includes and fixed cmake as well, PTAL.

@@ -1,20 +1,29 @@
## Process this file with automake to produce Makefile.in


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this extra new-line?

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Jun 16, 2015

LGTM

You might want to squash the commits into one.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

Now the Build tool needs to define -DHAVE_ZLIB and -DHAVE-PTHREAD rather
than providing a config.h

- Make pbconfig.h a manually written file to handle hash conditions
  according to platform related macros.
- Remove #include "config.h" from source code.
- Changed the configure.ac and Makefile.am to pass down the macros.
- Change cmake to pass down the the macros.

Change-Id: I537249d5df8fdeba189706aec436d1ab1104a4dc
@googlebot
Copy link

CLAs look good, thanks!

liujisi added a commit that referenced this pull request Jun 17, 2015
Check HAVE_CONFIG_H before including "config.h"
@liujisi liujisi merged commit b36395b into protocolbuffers:master Jun 17, 2015
@liujisi liujisi deleted the config_h branch June 17, 2015 02:12
taoso pushed a commit to taoso/protobuf that referenced this pull request Aug 1, 2018
This flag was necessary inside Google for the transition to having
proto3 preserve unknown fields by default.
Outside of Google, this is unnecessary since the dev branch is
the transition.
adellahlou pushed a commit to adellahlou/protobuf that referenced this pull request Apr 20, 2023
adellahlou pushed a commit to adellahlou/protobuf that referenced this pull request Apr 20, 2023
set resolvedResponseType on resolve(), fixes protocolbuffers#497
bithium pushed a commit to bithium/protobuf that referenced this pull request Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants