Skip to content

Disable PACKAGE_* preprocessor symbols #4020

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Disable PACKAGE_* preprocessor symbols #4020

wants to merge 3 commits into from

Conversation

@petk
Copy link
Member

@petk petk commented Apr 9, 2019

Hello, this is a fix for issue with phpize and redefined macros mentioned in #3967

@petk
Disable PACKAGE_* preprocessor symbols
cfff450 
Autoconf defines PACKAGE_* symbols:
- PACKAGE_NAME
- PACKAGE_VERSION
- PACKAGE_TARNAME
- PACKAGE_STRING
- PACKAGE_BUGREPORT
- PACKAGE_URL

and appends them to the generated config.h.in files. With AC_INIT change
via afd52f9 where package version, URL,
bug report location are defined, these preprocessor macros are then non
empty strings. When using phpize, PHP shares the config files in
extensions, warnings of redefined macros appear, such as:
- `warning: 'PACKAGE_NAME' macro redefined`

This patch now disables these non utilized symbols in the generated
config header files.

Better practice would be to include only API specific headers where
needed but this would require even more refactorings. Some extensions
such as pcre, pgsql, and pdo_pgsql solve this issue by undefining some
of these symbols before including the library configuration headers in
the code also. Because these symbols can be defined by any library which
uses Autotools.

Additionally, the unused PACKAGE_* symbols were cleaned for the bundled
libmbfl library.
@petk petk added the Quickfix label Apr 9, 2019
@nikic
Copy link
Member

nikic commented Apr 10, 2019

Can we just drop these from AC_INIT?

@petk
Copy link
Member Author

petk commented Apr 10, 2019 •
edited
Loading

Can we just drop these from AC_INIT?

They will be still defined in the php_config.h in any case...

That's why these need to be done basically also:

Otherwise, we can call just AC_INIT but this won't solve the main problem I think...

@nikic
Copy link
Member

nikic commented Apr 10, 2019

Can the two places you linked be dropped after this patch?

This looks like a pretty big hack, but I'm not sure what else can be done. https://stackoverflow.com/a/49548048/385378 also suggests that there is no good way to disable this...

@petk
Copy link
Member Author

petk commented Apr 10, 2019

Can the two places you linked be dropped after this patch?

Yes, all those can be dropped after this patch (pcre, pdo_pgsql, and pgsql).
(example has been done in the next commit, however one more undef needs to be added for the pg_config.h file to remove the warning: 'restrict' macro redefined - will be done in a separate commit because that is another problem not related to this).

But I agree, it is ugly patch. No solution here will be decent actually (only previous state is what PHP has in it already and no one notices it actually).

From my understanding here, with current versions and the state of Autoconf, there is no other option but to go into a custom hack such as this or using generated configuration headers a bit more consistently and carefully or having a customized predefined php_config.h.in template file in the Git repository.

These generated configuration headers seem to be designed and intended to be used a bit differently than what libraries and PHP do on some places. Each library is supposed to include its own (private/project level) config.h file only in *.c files without conflicting on one another when the headers of different libraries are included. Yes, it is a usual problem with defined symbols actually because namespacing things is not a practice in these libraries or some prefixing practices.

However, the problem will always be present no matter if changing configure.ac or not. With any library which is not including configuration headers in *.c files and is doing that in its api headers (such as for example pcre library and postgresql lib probably) the redefined warnings will show. For example, libfoo in the future which PHP might want to use by including its header file #include <libfoo.h> together with the pg_config.h. And if theoretically, PHP will mix the two defines in the same file in postresql or pcre part (for whatever reason we don't yet see at the moment), those redefined warnings will popup again and more undefs will need to be done for such case.

I'll leave the decision to you here:

a - clean config template (something like this patch) - I'll check if it can be done a bit nicer also
or
b - revert the afd52f9

nikic
nikic approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Okay, as this version allows to drop a bunch of undefs in various places, let's go with it.

configure.ac Outdated
# Disable PACKAGE_* symbols in main/php_config.h.in
$SED -e 's/^#undef PACKAGE_[^ ]*/\/\* & \*\//g' < $srcdir/main/php_config.h.in \
> $srcdir/main/php_config.h.in.tmp && \
mv $srcdir/main/php_config.h.in.tmp $srcdir/main/php_config.h.in
Copy link
Member

@nikic nikic Apr 11, 2019

Choose a reason for hiding this comment

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

Use sed -i instead?

Copy link
Member Author

@petk petk Apr 11, 2019

Choose a reason for hiding this comment

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

-i is not posix compliant option for sed :) Yes, I'd love to use the inplace directly also but it would need to be adjusted based on the system (BSD based and GNU based ones). Let me check...

Copy link
Member

@nikic nikic Apr 11, 2019

Choose a reason for hiding this comment

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

Ugh :( I guess it's fine then...

@petk
Copy link
Member Author

petk commented Apr 11, 2019

And additionally, another more logical option is to move the php_config.h.in and config.h.in cleanups to the buildconf and phpize step, so the processed files are shipped with the release directly.

So the commit 99e0cea moves all that to buildconf and phpize parts (approx):
php-build (1)

nikic
nikic approved these changes Apr 12, 2019
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Yes, this approach looks better... Nice graphic :)

@petk
Copy link
Member Author

petk commented Apr 13, 2019

Applied via 69b20f5 to PHP-7.4+.

@petk petk closed this Apr 13, 2019
@petk petk deleted the patch-package-symbols branch April 13, 2019 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikic nikic nikic approved these changes

Assignees

No one assigned

Labels

Quickfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@petk @nikic