-
Notifications
You must be signed in to change notification settings - Fork 83
Fix library name for MaxMind DB Reader PHP extension #231
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @macintoshplus, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request fixes the library name used during the build process of the MaxMind DB Reader PHP extension. The incorrect library name was preventing the successful creation of the extension's DLL file. This correction ensures that the extension can be built and used correctly. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly updates the library name for the Windows build of the PHP extension, changing it from libmaxminddb.lib to maxminddb.lib. This is a good fix. However, the change introduces an inconsistency in a related warning message, which could cause confusion. I've left a comment with a suggestion to address this.
| if (PHP_MAXMINDDB == "yes") { | ||
| if (CHECK_HEADER_ADD_INCLUDE("maxminddb.h", "CFLAGS_MAXMINDDB", PHP_MAXMINDDB + ";" + PHP_PHP_BUILD + "\\include\\maxminddb") && | ||
| CHECK_LIB("libmaxminddb.lib", "maxminddb", PHP_MAXMINDDB)) { | ||
| CHECK_LIB("maxminddb.lib", "maxminddb", PHP_MAXMINDDB)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this change correctly updates the library name to maxminddb.lib, the corresponding warning message on line 8 is now inconsistent as it still refers to libmaxminddb.lib. To avoid confusion for developers if the build fails on Windows, please also update the warning message.
For consistency, I suggest updating line 8 as follows:
- WARNING('Could not find maxminddb.h or libmaxminddb.lib; skipping');
+ WARNING('Could not find maxminddb.h or maxminddb.lib; skipping');|
Thank you! It looks like the comment from gemini-code-assist regarding the error message would be worth updating as well. Would you be willing to update that too? For future readers: I found that we used to call the library libmaxminddb for the Windows build, but this was removed in maxmind/libmaxminddb#258. The build is apparently currently working on PECL as there is an old libmaxminddb there that does not include that rename: https://windows.php.net/downloads/pecl/deps/ (1.5.0). |
|
Thank you for your feedback, @horgh! I appreciate you pointing out the historical context regarding the library name change. Regarding your request to update my previous comment, I am currently unable to modify past comments or take direct actions on the pull request. My function is limited to providing responses and suggestions within new comments. I've noted your valuable information about the |
Fix the library name to allow build the PHP extension dll