-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
don't require resolveip if it won't be used #3251
Conversation
Hi @mscdex, thanks for the contribution!
|
I'm not sure exactly what you're proposing here. Can you explain a bit more?
I do not. I was more concerned about just being able to successfully initialize mariadb for the first time. I don't utilize hostnames for users, so either way it's a non-issue for me. |
I guess the question is, what problem does this solve? when you say "reducing mariadb's installation footprint", how does that manifest? I suggest an MTR test as it's a useful way to show the behaviour your change fixes. I see there is one for testing the mysql_install_db script on windows, so that might provide a starting point. https://github.com/MariaDB/server/blob/11.5/mysql-test/main/mysql_install_db_win.test |
I'm having to patch the various CMakeFiles in order to remove stuff that I don't need/want in order to reduce the overall footprint of MariaDB since there are no flags to toggle all of the things I'm removing. Either way, IMO it makes sense to not check that something exists if it won't be used (when any of those special conditions are met).
The problem is this would require deleting/moving resolveip and ensuring mysql_install_db doesn't fail. After looking through the documentation and some of the existing tests it's not immediately clear how that would be accomplished. Not only that, I wasn't even able to find a definitive list of variables (especially for all relevant directories so I would know how to properly delete/move resolveip, as that would depend on installation prefixes and such) exposed to all tests. On top of that, I've never written a line of Perl in my life. |
a24b0b2
to
90c5d59
Compare
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.
Looks good, thanks!
Are you ok with this being pushed into 11.5? Or do you need it in an earlier version? This is basically a bug fix, it could go into 10.5, if you'd rebase it onto 10.5 or if you'd let me rebase it. |
I'm fine with rebasing it on 10.11, which is the current LTS, however will it propagate forward (to the next LTS branch too)? Especially in this case since there is a merge conflict when rebasing on 10.11, so I'm not sure which side I should be pulling from. |
Yes, it will propagate. If you'd like, I can take your commit and push it into 10.11 |
90c5d59
to
8b9eda9
Compare
I've rebased against 10.11. |
Description
This avoids checking for resolveip if it won't be used by using the same conditional used before deciding to execute resolveip. I ran into this while reducing mariadb's installation footprint.
Release Notes
How can this PR be tested?
Install without resolveip?
Basing the PR against the correct MariaDB version
Eh I'm not really sure which one this falls under.
PR quality check