pcp icon indicating copy to clipboard operation
pcp copied to clipboard

pcp/pmapi.h pollutes global namespace and conflicts with other projects' config.h

Open Explorer09 opened this issue 1 year ago • 4 comments

This issue was found when building htop with PCP. The issue was discussed here (htop-dev/htop#1337), but I think it's good to mention this upstream.

<pcp/pmapi.h> internally includes PCP's config.h, which is generated by autoheader and is installed alongside the compiled libpcp. An autoheader-generated config header should not be installed as a library header as that would pollute the global namespace with unnecessary macro defines. For example, some htop headers have to undef the following to avoid name conflicts with PCP:

#undef PACKAGE_NAME
#undef PACKAGE_TARNAME
#undef PACKAGE_VERSION
#undef PACKAGE_STRING
#undef PACKAGE_BUGREPORT
#undef PACKAGE_URL

A PCP header, when installed as a library header, should either be self contained or depend only on macros that are properly prefixed (PCP_HAVE_*, etc).

I see that <pcp/pmapi.h> has conditional includes of certain system headers. Those conditional includes should probably be replaced with hard-coded include lines when pmapi.h is installed as a library header. I didn't track other dependencies of pmapi.h on config.h yet.

Explorer09 avatar Dec 11 '23 13:12 Explorer09

I am not sure these PACKAGE_ macros even need to be in config.h ... they are needed in builddefs for the build, but I don't see any use of them, or any PACKAGE_ macro, in the C source code below src nor below qa, so I think they can be removed from config.h.in and hence config.h.

The other parts of the issue are not so easily fixed ... 8^(

kmcdonell avatar Dec 11 '23 22:12 kmcdonell

And builds for both Ubuntu 22.04 and Fedora 38 both complete without issues after the offending lines are removed from config.h.in. I'll consult with @natoscott to make sure I've not missed something, and if not, consider the PACKAGE_ macros gone from config.h

kmcdonell avatar Dec 12 '23 00:12 kmcdonell

I thought PCP's config.h.in file was generated by autoheader (as mentioned in the first line comment of the file) and so is unsafe to edit directly. The next invocation of autoreconf would overwrite it, wouldn't it?

If config.h.in was maintained manually. Perhaps you guys should remove that comment line (and of course make sure autoreconf or whatever your bootstrap script is won't touch it). I also believe config.h.in can probably strip further with all unnecessary macros removed.

Explorer09 avatar Dec 12 '23 06:12 Explorer09

config.h.in is not generated by autoheader (if may have been at some distant point in the past) ... I've amended the comment at the head of the file.

All of the macros in config.h.in (which becomes config.h after configure has chomped on it) are needed to provide the glue between configure and the rest of the PCP source code ...

  • 235 of them are of the form HAVE_FOO for library or header conditional bits
  • 12 of them are typedefs or compiler keywords that are not available on all platforms
  • 9 of them are of the form IS_FOO for platform/OS/... variants
  • 6 of them are "odds and sods" for various things

I don't see anything there that is "unnecessary" and the vast majority of the names should not cause any namespace collisions ... but please let me know if you see something to the contrary.

kmcdonell avatar Jan 14 '24 22:01 kmcdonell

I think we're done with this issue. @Explorer09 if there are remaining namespace conflicts, please open a new issue with the details. Thanks.

kmcdonell avatar Apr 09 '24 07:04 kmcdonell