224698 - remove localconfig variable $mysqlpath

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Jouni Heikniemi]

If I understand correctly, $mysqlpath in localconfig is no longer necessary,
since bug 180870 removed the old sync code. Do we still need that? 

If we decide to get rid of locating mysql binaries, it'll make fixing bug 143490
a bit easier (since we don't have to replace unix |where| with the appropriate
win32 replacement).

Comment 1

[chaduv]

Sounds ok to me. It ($mysqlpath) probably should have been removed in the bug 
180870 cleanup. I'll create the trivial patch tonight if no one else does.

Comment 2

[chaduv]

Attachment: remove mysqlpath from checksetup

Tested, no seen problems. I don't use DB replication, so someone else might
want to take a whack.

Comment 3

[Dave Miller [:justdave]]

I'm pretty sure the DB replication is all done server side now.  Bugzilla itself
has no reason to know where MySQL is now (aside from a host and port number)

Comment 4

[Christian Reis]

Comment on attachment 134797 [details] [diff] [review]
remove mysqlpath from checksetup

Might be nice to tell people that localconfig doesn't use that variable any
more if it's left in there, but adding that check to checksetup.pl is up to you
now :)

Comment 5

[chaduv]

Attachment: Comment to user that mysqlpath is gone

Whichever way is wanted.

Comment 6

[chaduv]

Wrote patch, taking.

Comment 7

[Dave Miller [:justdave]]

hmmm, I like the second one...

but the comment it gives the user seems incomplete and/or funny sounding.

Anyone have recommendations for what we should tell the user?

"The \$mysqlpath variable in localconfig is no longer needed.  You may delete it
if you like, but leaving it there won't cause any harm (other than having to
read this message)."   ? :)

Comment 8

[Jouni Heikniemi]

"The \$mysqlpath setting in your localconfig file is no longer necessary. It is
recommended to remove it."

Although it's more truthful to say it doesn't harm anything, the meaning of the
warning message is pretty vague if it just says "you can do that or you can
leave it undone". Well, if that's so irrelevant, why whine about it? 

Recommending removal would perhaps in the long term clean up configuration
files, which is always good -- at least nobody will then think their db
connection problem is caused by the $mysqlpath setting.

Comment 9

[Dave Miller [:justdave]]

clearing approval request pending answering the remaining questions/getting a
new patch posted.

Comment 10

[Gervase Markham [:gerv]]

There's an existing mechanism for removing variables from localconfig, IIRC. It
dumps them in a text file and prints a message when you run checksetup.pl the
next time. See if you can find it and work out how it works.

Gerv

Comment 11

[Jouni Heikniemi]

I thought we had that only for params?

Comment 12

[Dave Miller [:justdave]]

Nope, that's only for params.  We've never had a way to remove stuff from
localconfig yet.  It only works for params because the whole file is rewritten
from scratch.  Localconfig gets appended to when we add stuff.

This is the reason the release notes say that putting comments in localconfig is
deprecated because they may get overwritten in some future code.  Because we
planned to overwrite it at some point...

Comment 13

[Gervase Markham [:gerv]]

Oh, OK. Sorry.

But perhaps now is a good time to write said generic mechanism, along the same
lines?

Gerv

Comment 14

[Dave Miller [:justdave]]

OK, for 2.18, lets get a new patch up that recommends removing it.

We should already have another bug somewhere for a more logical handling of
localconfig, if not, let's file one and do that later.

Comment 15

[Dave Miller [:justdave]]

Comment on attachment 134816 [details] [diff] [review]
Comment to user that mysqlpath is gone

per comment 7.

Can we get the patch refreshed to deal with that?

Comment 16

[Vlad Dascalu]

Attachment: v3

Comment 17

[Christian Reis]

Comment on attachment 145375 [details] [diff] [review]
v3

No-brainer.

"It is recomended to remove it" -> "It is recomended you remove it" perhaps.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

>"It is recomended to remove it" -> "It is recomended you remove it" perhaps.

"We recommend you remove it." (active tense better than passive tense, even with
royal we)

But that's icing.  a=myk with or without the change

Comment 19

[Vlad Dascalu]

Checked in with the proposed rewording.

FIXED.

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.273; previous revision: 1.272
done