mysql: improve warnings when data directory is present. #60190

peterb · 2020-08-26T10:50:19Z

Users are already warned about config files, warn users about conflicting data directory too.

MikeMcQuaid

Thanks for the PR! I think the [WARNING] is a bit out of place and a few questions/tweaks but otherwise: nice work!

Formula/mysql@5.7.rb

dtrodrigues · 2020-08-26T12:46:33Z

Can you prefix the commit message with mysql@5.7: please so it's clear that this is specific to mysql and not homebrew in general?

peterb · 2020-08-26T13:32:34Z

I've removed the warning, but this won't detect the presence of that data directory unless it can add something to the caveats from the post_install method, is this possible?

Update: I don't think that is possible, but at least users will get a note to remind them that this data directory could cause issues.

MikeMcQuaid · 2020-08-27T07:29:10Z

I've removed the warning

Thanks!

but this won't detect the presence of that data directory unless it can add something to the caveats from the post_install method, is this possible?

I'm not sure I understand this, can you explain why this would be useful?

Update: I don't think that is possible, but at least users will get a note to remind them that this data directory could cause issues.

Yes, I was thinking this alone would be useful.

peterb · 2020-08-27T14:03:56Z

I'm not sure I understand this, can you explain why this would be useful?

We could detect the presence of the data directory before the post_install runs and alter the caveats depending on the result, so the user could be reminded that they had already installed mysql there. I believe that when the user invokes the formula the install method runs, then the post_install method, and then the caveats method. When the program enters the post_install method, there isn't any way of modifying the caveats, but it's at this point when we could check to see if that data directory already exists and add a warning about it for the user to see. Once the program is in the caveats method, it's already too late to check if there was a pre-existing data directory as brew has already run the mysql initialize on it via the post_install method.

MikeMcQuaid · 2020-08-27T15:02:10Z

I see. In that case the change as-is seems rather pointless, unfortunately. Can you point to the specific issues that are being caused by this being already present and how failures occur?

peterb · 2020-08-27T15:15:44Z

Yes, I added some details to before I opened this issue on https://discourse.brew.sh/t/for-homebrew-mysql-installs-how-to-fix-mysql-sock-path/660/8

MikeMcQuaid · 2020-08-27T15:29:46Z

I see, thanks.

Perhaps the install task should check if the data directory is present and abort if it is rather than silently failing?

I think in this case it needs to check specifically if the data directory exists and is for a newer version of MySQL than the one you're installing. I'm not sure how to do this but it feels like the right direction (and would need to be made to mysql.rb and all mysql@*.rb formulae).

gromgit · 2020-08-27T15:41:04Z

@peterb You may want to look at how the postgresql formula deals with DB upgrades. In short, its caveat references an external Homebrew command postgresql-upgrade-database that users can run to Do The Right Thing. The source is in $(brew --repo homebrew/core)/cmd/postgresql-upgrade-database.rb.

MikeMcQuaid · 2020-08-27T15:51:48Z

@gromgit This isn't necessary/comparable with MySQL because:

MySQL will automatically upgrade databases without intervention whereas PostgreSQL requires a manual command to be run
PostgreSQL uses separate data directories per-version and MySQL (how we have it setup, at least) does not.

We could consider using separate data directories for the older versions of MySQL we have (but we'd need to have it only take effect if the datadir doesn't exist so we don't break existing installs).

peterb · 2020-08-28T14:10:18Z

@MikeMcQuaid I'm happy to look at the mysql@*.rb formulae so that they create and use separate data directories unless there's already one present for the specific version the user is installing. I'm assuming there is a way to detect which version of MySQL a data directory corresponds to, and that the mysql formula won't require a check as it'll auto-upgrade if there was data from an older version present?

MikeMcQuaid · 2020-08-28T15:13:08Z

I'm assuming there is a way to detect which version of MySQL a data directory corresponds to

Yeh, there should be. I have no idea what it is, though 😭

that the mysql formula won't require a check as it'll auto-upgrade if there was data from an older version present?

Yes, that's a safe assumption 👍🏻

MikeMcQuaid

Just so my review is here (as @SMillerDev requested this): as-is this unfortunately won't do anything but we could consider moving the datadir for new installs.

peterb · 2020-09-02T08:17:39Z

@MikeMcQuaid I'm looking into this already, have just pushed some of the changes.

MikeMcQuaid · 2020-09-02T08:35:46Z

Thanks @peterb, no rush!

Users are already warned about config files, warn users about conflicting data directory too.

Refine logic for detecting which version of MySQL the datadir corresponds to by refactoring and adding heuristic for detection of MySQL 5.6.

Use special installation directory in MySQL 5.6 formula as well, unless the default is already being used.

MikeMcQuaid · 2020-09-09T07:49:08Z

Formula/mysql@5.6.rb

+  # type MYISAM, MySQL version 50647
+  #
+  def default_datadir_already_in_use_by_this_version?
+    (var/"mysql").exist? && var.glob("mysql/*.dblwr").empty? && (var/"mysql/test").exist?


Think it's enough to just check for (var/"mysql").exist??

Could also output something in the caveats based on this?

Sounds good, I think that a simpler check and a caveat explaining the location of the datadir to the user would be preferable to trying to work it out automatically for them. I'll modify as you suggest.

Update: It turns out this is harder to do than I expected, as putting a conditional in the datadir method means it gets evaluated differently during various stages in the formula.

Formula/mysql@5.7.rb

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

for each version.

…o patch-2

peterb · 2020-09-11T13:11:11Z

@MikeMcQuaid I've updated this to something along the lines of what you were suggesting, I think that detecting if another version of MySQL is installed and using a different datadir is a nice idea in theory, but based on how the formula works it could make it more confusing for the user, and detecting the version of MySQL isn't going to be reliable because there are so many different heuristics but no definitive way of doing it.

With separate datadirs for each version (mysql57 and mysql56 are just suggestions), the possibilities of a conflict should be ameliorated quite a bit, as they can run brew install mysql; brew install mysql@5.7 and use any of these versions by running brew services stop mysql; brew services start msql@5.7 for people that have to deal with legacy databases, this should make using Homebrew to install them a lot friendlier.

Summary: assuming a fresh OSX install, this would get rid of the gotcha that 5.7 will break and requires manually fixing the datadir if MySQL 8 is already installed. If they already have a legacy MySQL installed and decide to run reinstall, then the caveats will tell them that the datadir location has moved.

peterb · 2020-09-22T14:13:43Z

Not sure what the consensus is on this one, so closing it for the moment.

MikeMcQuaid · 2020-09-22T14:24:23Z

Reopening because it's on my list, sorry for the delay here.

MikeMcQuaid · 2020-11-25T10:38:41Z

@peterb #65102 demonstrates the sort of approach I think would make sense to follow for MySQL. Let me know if you have any thoughts/questions but hopefully this will unblock you.

peterb · 2020-12-07T10:19:40Z

@MikeMcQuaid thanks! At a first glance #65102 looks like a good example, I'll take a more detailed look at it.

peterb · 2020-12-11T17:34:52Z

@MikeMcQuaid MySQL 5.6 has reached end-of-life, so it's probably only worth updating the formula for MySQL 5.7. EDIT: 5.6 will reach EOL in three months time.

BrewTestBot · 2021-01-02T01:04:59Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

BrewTestBot · 2021-01-24T01:32:28Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

peterb · 2021-01-28T12:23:15Z

@MikeMcQuaid @SMillerDev @gromgit sorry for the delay in picking his up. I could look at MySQL 5.7? How valuable would be for Homebrew?

SMillerDev · 2021-01-28T12:29:01Z

Go for it, every improvement is valuable

BrewTestBot · 2021-02-19T00:46:23Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

BrewTestBot added deprecated license Formula uses a deprecated SPDX license which should be updated legacy Relates to a versioned @ formula labels Aug 26, 2020

MikeMcQuaid reviewed Aug 26, 2020

View reviewed changes

Formula/mysql@5.7.rb Outdated Show resolved Hide resolved

Formula/mysql@5.7.rb Outdated Show resolved Hide resolved

Formula/mysql@5.7.rb Outdated Show resolved Hide resolved

SMillerDev changed the title ~~Improve warnings when data directory is present.~~ mysql: improve warnings when data directory is present. Aug 26, 2020

peterb force-pushed the patch-2 branch from 14149eb to 0a1a7ce Compare August 26, 2020 13:29

SMillerDev requested a review from MikeMcQuaid September 1, 2020 21:20

MikeMcQuaid requested changes Sep 2, 2020

View reviewed changes

peterb force-pushed the patch-2 branch from 62cc759 to 0ce0860 Compare September 6, 2020 10:42

peterb added 8 commits September 6, 2020 17:16

mysql@5.7: Improve warnings when data directory is present.

75c610a

Users are already warned about config files, warn users about conflicting data directory too.

Pseudocode of what this should do.

b9194c5

Add note about 5.6 changes which are also required.

4497d80

Move pseudocode to comments and flesh out some working code.

f5c2c71

Detect MySQL 5.6, refactor.

943133e

Refine logic for detecting which version of MySQL the datadir corresponds to by refactoring and adding heuristic for detection of MySQL 5.6.

Fix style issues.

a5ce548

MySQL 5.6 formula installation directory.

6992d7a

Use special installation directory in MySQL 5.6 formula as well, unless the default is already being used.

Remove pseudocode.

ab78b97

peterb force-pushed the patch-2 branch from ad690d9 to ab78b97 Compare September 6, 2020 16:17

peterb force-pushed the patch-2 branch from 0516968 to 93b8b07 Compare September 8, 2020 20:00

MikeMcQuaid reviewed Sep 9, 2020

View reviewed changes

peterb and others added 4 commits September 9, 2020 18:46

Update Formula/mysql@5.7.rb

e6936de

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

Simply legacy mysql formulae by specifying a separate data directory

2dd4960

for each version.

Merge branch 'patch-2' of https://github.com/peterb/homebrew-core int…

94610a2

…o patch-2

Remove custom socket as it appears that the CI error is unrelated.

d61101d

peterb closed this Sep 22, 2020

MikeMcQuaid reopened this Sep 22, 2020

MikeMcQuaid mentioned this pull request Nov 18, 2020

PostgreSQL@12: Use its own data dir and log files when run via services #65102

Merged

5 tasks

This was referenced Dec 11, 2020

Test and build failures found during python 3.9 testing #66450

Closed

mariadb*, mysql*, percona-server: fix CI test issues #66727

Closed

BrewTestBot added stale No recent activity and removed stale No recent activity labels Jan 2, 2021

BrewTestBot added the stale No recent activity label Jan 24, 2021

BrewTestBot removed the stale No recent activity label Jan 28, 2021

BrewTestBot added the stale No recent activity label Feb 19, 2021

BrewTestBot closed this Feb 26, 2021

github-actions bot added the outdated PR was locked due to age label Apr 6, 2021

github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mysql: improve warnings when data directory is present. #60190

mysql: improve warnings when data directory is present. #60190

peterb commented Aug 26, 2020

MikeMcQuaid left a comment

dtrodrigues commented Aug 26, 2020

peterb commented Aug 26, 2020 •

edited

Loading

MikeMcQuaid commented Aug 27, 2020

peterb commented Aug 27, 2020

MikeMcQuaid commented Aug 27, 2020

peterb commented Aug 27, 2020

MikeMcQuaid commented Aug 27, 2020

gromgit commented Aug 27, 2020

MikeMcQuaid commented Aug 27, 2020

peterb commented Aug 28, 2020

MikeMcQuaid commented Aug 28, 2020

MikeMcQuaid left a comment

peterb commented Sep 2, 2020

MikeMcQuaid commented Sep 2, 2020

MikeMcQuaid Sep 9, 2020

MikeMcQuaid Sep 9, 2020

peterb Sep 9, 2020

peterb Sep 9, 2020

peterb commented Sep 11, 2020

peterb commented Sep 22, 2020

MikeMcQuaid commented Sep 22, 2020

MikeMcQuaid commented Nov 25, 2020

peterb commented Dec 7, 2020

peterb commented Dec 11, 2020 •

edited

Loading

BrewTestBot commented Jan 2, 2021

BrewTestBot commented Jan 24, 2021

peterb commented Jan 28, 2021

SMillerDev commented Jan 28, 2021

BrewTestBot commented Feb 19, 2021

mysql: improve warnings when data directory is present. #60190

mysql: improve warnings when data directory is present. #60190

Conversation

peterb commented Aug 26, 2020

MikeMcQuaid left a comment

Choose a reason for hiding this comment

dtrodrigues commented Aug 26, 2020

peterb commented Aug 26, 2020 • edited Loading

MikeMcQuaid commented Aug 27, 2020

peterb commented Aug 27, 2020

MikeMcQuaid commented Aug 27, 2020

peterb commented Aug 27, 2020

MikeMcQuaid commented Aug 27, 2020

gromgit commented Aug 27, 2020

MikeMcQuaid commented Aug 27, 2020

peterb commented Aug 28, 2020

MikeMcQuaid commented Aug 28, 2020

MikeMcQuaid left a comment

Choose a reason for hiding this comment

peterb commented Sep 2, 2020

MikeMcQuaid commented Sep 2, 2020

MikeMcQuaid Sep 9, 2020

Choose a reason for hiding this comment

MikeMcQuaid Sep 9, 2020

Choose a reason for hiding this comment

peterb Sep 9, 2020

Choose a reason for hiding this comment

peterb Sep 9, 2020

Choose a reason for hiding this comment

peterb commented Sep 11, 2020

peterb commented Sep 22, 2020

MikeMcQuaid commented Sep 22, 2020

MikeMcQuaid commented Nov 25, 2020

peterb commented Dec 7, 2020

peterb commented Dec 11, 2020 • edited Loading

BrewTestBot commented Jan 2, 2021

BrewTestBot commented Jan 24, 2021

peterb commented Jan 28, 2021

SMillerDev commented Jan 28, 2021

BrewTestBot commented Feb 19, 2021

peterb commented Aug 26, 2020 •

edited

Loading

peterb commented Dec 11, 2020 •

edited

Loading