Re: [Libguestfs] [PATCH 6/7] python: stop including config.h

On Mon, Jan 13, 2020 at 10:35:53AM +0100, Pino Toscano wrote: > On Friday, 10 January 2020 23:13:35 CET Richard W.M. Jones wrote: > > On Thu, Jan 09, 2020 at 06:37:49PM +0100, Pino Toscano wrote: > > > There is nothing in the Python bindings that require results from > > > configure, and gnulib is not used already. > > > --- > > > generator/python.ml | 6 ------ > > > python/handle.c | 2 -- > > > 2 files changed, 8 deletions(-) > > > > I guess the motivation is because we cannot create this header > > correctly from PyPi so it'll be wrong there? > > That is the thing: config.h is not created at all when building the > Python binding (either as part of libguestfs, or when building the > standalone tar.gz dist), but instead the one from libguestfs is used. So for clear terminology let's call these situations: - libguestfs build: Python bindings are built normally as part of libguestfs - PyPi build: Python bindings are built from the python-specific tar gz file For the libguestfs build case we create config.h from ./configure. For the PyPi build case we bundle config.h (which is wrong - not arguing with that). Because in the PyPi case there is no real equivalent of ./configure we cannot create config.h properly. > > I still think we might > > need this header when compiling libguestfs Python bindings in the > > normal (not PyPi) case, even if it happens to work right now. > > Actually, it happens to _not_ work right now: > https://bugzilla.redhat.com/show_bug.cgi?id=1627964 > the libguestfs config.h is shipped in the tarball, and the content of > that file depends on the way the binding was configured from the > libguestfs build that created it. Yup, this is wrong, not arguing there. > So in case of that bug, the tarball > was created with Python 2. And while patches #3, #4, and #5 should fix > the majority of the visible cases of this issue, there are still > potential issues: config.h defines a series of macros (_GNU_SOURCE, > __EXTENSIONS__, etc) that change the way things are built, and strictly > depend on the platform. > > > Could we instead defend these with #ifdef HAVE_CONFIG_H? That way the > > header shouldn't be used for PyPi and shouldn't need to be bundled (so > > patch #7 would be OK). > > I'll reverse the question: if building the standalone tarball works fine > with no config.h, why shouldn't the build-in-libguestfs do the same? The Python bindings probably don't need the contents of config.h at the moment, but that isn't to say that config.h might not be necessary on some more obscure platform either now or in the future.

In the libguestfs build case using HAVE_CONFIG_H would ensure this keeps working, while also preventing us from needing to bundle the incorrect config.h for the PyPi build case. I think it achieves what you're trying to do (fix the PyPi case) with least risk.