Comment 1 Mahdi Mokhtari 2016-05-17 14:03:12 UTC

Things i guess matters for us are (but not closed to ;D) : 
1)respect hier(7)
2)not surprise users
3)respect mysql original decisions/updates

Created attachment 170407 [details]
better inform users about new default location for my.cnf

Proposed patch for your point #2

Comment 3 Mahdi Mokhtari 2016-05-17 14:26:39 UTC

(In reply to Markus Kohlmeyer from comment #2)
my.cnf Path is wrong right now ? (i assumed when we was working on mysql upgrade to 5.7.12, we corrected things).

I thought that's why we warned ppl for future changes in pkg-message

Comment 4 Mahdi Mokhtari 2016-05-17 14:27:13 UTC

(In reply to Mahdi Mokhtari from comment #3)
I just asked this cause you changed mysql-server.in

my.cnf path is and was correct, i just reworded the warning/hint to be more clear to users and added the missing UPDATING entry.

Created attachment 170408 [details]
Backport of databases/mariadb101-server/files/patch-mysys_my__default.c

(In reply to Mahdi Mokhtari from comment #1)

Proposed patch for your point #1

Backport of databases/mariadb101-server/files/patch-mysys_my__default.c

(In reply to Mahdi Mokhtari from comment #1)
Can you explain your point #3 a bit more please?

(In reply to Markus Kohlmeyer from comment #6)

Forgot the new output for reference:


[root@devnoip:~] # /usr/local/libexec/mysqld --verbose --help | grep -A1 'Default options'
Default options are read from the following files in the given order:
/usr/local/etc/my.cnf /usr/local/etc/mysql/my.cnf ~/.my.cnf

Comment 9 Mahdi Mokhtari 2016-05-17 15:24:00 UTC

(In reply to Markus Kohlmeyer from comment #5)
Okay i'm okay with this. just few point before we have it approved ;)

Nothing practically changes (in Paths, ...), correct ?

Is it necessary to warn ppl every time service starts ?  (i'm talking about "if [ -f "/usr/local/etc/my.cnf" ]; then")

I think previous (current) wording of pkg-message is better. (as if we say path is gonna change they make themselves ready, but if we say "things [has been] changed" they will be surprised ;D)

What about just adding entry to UPDATING only ?


(In reply to Markus Kohlmeyer from comment #6)
Did you tested your back-port ?
i expected to have this for mysql57-server not client :thinking:


(In reply to Markus Kohlmeyer from comment #7)
point #3 is main reason of issue/PR, when mysql does a change as upstream, we plan to have their changes (updates) respecting FreeBSD env. (instead of ignoring their change and maintaining patches for ignoring them :D)

We can stay with the previous wording or improve it even more, as you like ;)

For me UPDATING is always enough, but i know a few sysadmins that never read UPDATING (and pkg-message is often flying by unread, espacially when using tools like portupgrade/portmaster to manage and/or bulk update ports) so these will miss the info. Therefor rc-script seems to me like a legit option, even when annoying some users including myself. I'm open for other solutions and even for removing it from rc-script

(In reply to Mahdi Mokhtari from comment #9)
Yes i did a build on 10.3-RELEASE amd64 and short runtime check, both OK on my sytem.

> Nothing practically changes (in Paths, ...), correct ?

Correct.

Comment 13 Mahdi Mokhtari 2016-05-17 15:53:52 UTC

(In reply to Markus Kohlmeyer from comment #10)
> We can stay with the previous wording or improve it even more, as you like ;)
Thanks so much :D if decision is on me with wording (in metric of how much ppl being shocked), i definitely prefer previous wording, it's soft and kind word of wise advice ;D


> For me UPDATING is always enough, but i know a few sysadmins that never read UPDATING (and pkg-message is often flying by unread, espacially when using tools like portupgrade/portmaster to manage and/or bulk update ports) so these will miss the info. Therefor rc-script seems to me like a legit option, even when annoying some users including myself. I'm open for other solutions and even for removing it from rc-script
I definitely respect your experience as an expert sysadmin (and my preference is using it as much as possible).
If you met ppl who wont be aware of event by (not-)reading UPDATING and pkg-message. i personally think we should make them aware in any way we can (including service file). for annoying part of huge message, i guess we can keep message as short as possible with small message (using a wording like current pkg-message, in point of kind/soft advice of warning that in FUTURE thing will change)


> Yes i did a build on 10.3-RELEASE amd64 and short runtime check, both OK on my sytem.
Thanks for your confirming (and i count it as part of my QA on building), just let me do little linting QA too ;)

Comment 14 Mahdi Mokhtari 2016-05-17 16:41:09 UTC

(In reply to Markus Kohlmeyer from comment #13)
I think it's better we join this backported-patch with mysql57-server's existing one.
What do you think about it ?

Comment 15 Mahdi Mokhtari 2016-05-17 16:44:55 UTC

(In reply to Mahdi Mokhtari from comment #14)
never mind :D
ignore my previous comment ;)
Both client and server parts are needed ;)

Comment 16 Mahdi Mokhtari 2016-05-17 17:06:53 UTC

Comment on attachment 170408 [details]
Backport of databases/mariadb101-server/files/patch-mysys_my__default.c

Lint and Build* are seem to be Okay.
* on 10 and 11, still testing for 9 :D

Comment 17 Mahdi Mokhtari 2016-05-17 17:17:33 UTC

(In reply to Mahdi Mokhtari from comment #16)
built successfully done for FBSD9 too ;)
QA done successfully.

Attachment https://bugs.freebsd.org/bugzilla/attachment.cgi?id=170408 is ready to be committed in IMO ;)

Created attachment 170417 [details]
remove -DSYSCONFDIR from CMAKE_ARGS


Remove -DSYSCONFDIR from CMAKE_ARGS to avoid possible conflicts with default paths for my.cnf and keyring file and maybe others too


Sorry, forgot it in previous patch

Comment 19 Mahdi Mokhtari 2016-05-18 10:52:10 UTC

(In reply to Markus Kohlmeyer from comment #18)
as i remember from removing -DSYSCONFDIR it causes using `/etc/*` yes?
Or with patch that we have backported it will fixed ?
I'm asking this cause of point#1 :D

(In reply to Mahdi Mokhtari from comment #19)

> as i remember from removing -DSYSCONFDIR it causes using `/etc/*` yes?

No, but it allows again using /usr/local/etc/my.cnf as it was in previous versions and also is with MariaDB and Percona.

> Or with patch that we have backported it will fixed ?

The patch will remove /etc from the internal search patch for my.cnf and warn the user accordingly. Thats what makes us comply with hier(7) and is the same behavior MariaDB has for some time now.

Comment 21 Mahdi Mokhtari 2016-05-18 11:36:24 UTC

(In reply to Mahdi Mokhtari from comment #19)
Yeah i see ;) thanks for clarifying things ;D
Then let me merge these two patches in one patch-file (as aim is one)

Comment 22 Mahdi Mokhtari 2016-05-18 11:46:07 UTC

Created attachment 170435 [details]
merging two patches about my.cnf place

QA previously done Okay.
Patch is ready to commit.

Comment 23 Mahdi Mokhtari 2016-05-18 11:46:32 UTC

Thanks Markus ;)

Please don't forget the UPDATING entry ;)

Comment 25 Mahdi Mokhtari 2016-05-18 12:59:33 UTC

(In reply to Mahdi Mokhtari from comment #23)
I was waiting for you to reword the necessary things :)

Same wording as in pkg-message and rc-script

diff -Naur UPDATING UPDATING
--- UPDATING	2016-05-17 14:02:17.527385000 +0200
+++ UPDATING	2016-05-17 14:44:07.167788000 +0200
@@ -5,6 +5,15 @@
 You should get into the habit of checking this file for changes each time
 you update your ports collection, before attempting any port upgrades.
 
20160517:
  AFFECTS: users of databases/mysql57-(server|client)
  AUTHOR: rootservice@gmail.com

  Please keep in mind that the default location for my.cnf will be changed
  from "/var/db/mysql/my.cnf" to "/usr/local/etc/mysql/my.cnf" in the near
  future.  If you do not want to move your my.cnf to the new location then
  you must set "mysql_optfile" in /etc/rc.conf to "/var/db/mysql/my.cnf".

 20160511:
   AFFECTS: Users of audio/clementine-player
   AUTHOR: sbruno@FreeBSD.org

Created attachment 170441 [details]
UPDATING entry

Ignore previous comment.

As regular patch now.

Comment 28 Mahdi Mokhtari 2016-05-18 15:18:49 UTC

Comment on attachment 170441 [details]
UPDATING entry

Thanks for rewording
Markus ;)

Comment 29 Mahdi Mokhtari 2016-05-18 15:19:46 UTC

(In reply to Mahdi Mokhtari from comment #28)
Ready to be committed in IMO :)

Comment 30 Mahdi Mokhtari 2016-05-28 17:15:02 UTC

DeAssigning from myself so that a committer comes and takes it faster :)

Comment 31 Thomas Zander 2016-06-25 14:30:39 UTC

Is this up to date since mysql has been updated to new upstream versions?

Comment 32 Mahdi Mokhtari 2016-06-25 15:16:27 UTC

(In reply to Thomas Zander from comment #31)
Not all of them updated.
I guess i should regenerate patches that are working with Makefile.
UPDATING entry patch can be committed though (doesn't need to be regenerated IMO :D)

Comment 33 Mahdi Mokhtari 2016-06-25 15:23:46 UTC

Created attachment 171791 [details]
merging two patches about my.cnf place (updated)

Updated this patch for solving conflicts about PORTREVISION.
**it was small conflict caused by updating port to 5.7.13 and resetting PORTREVISION to 0

Comment 34 Mahdi Mokhtari 2016-06-25 15:24:58 UTC

(In reply to Thomas Zander from comment #31)
@Thomas Zander
Done :)
Both patches are now updated and ready to commit in ;)

Comment 35 Thomas Zander 2016-06-25 18:38:33 UTC

I have some (minor) questions:
1) The UPDATING entry is still correct? So the port does still follow the old behaviour by default for now?
2) The patch adds hardcoded "/usr/local" strings at some points. This is probably not an issue for package building, but what about the users who operate systems with a custom prefix? The port is actually quite close to work with custom prefix as it seems from the build log :)

Comment 36 Thomas Zander 2016-06-25 19:03:23 UTC

Also, did you see this one in poudriere testport:
====> Running Q/A tests (stage-qa)
====> Checking for pkg-plist issues (check-plist)
===> Parsing plist
===> Checking for items in STAGEDIR missing from pkg-plist
Error: Orphaned: lib/mysql/plugin/keyring_udf.so
Error: Orphaned: lib/mysql/plugin/test_udf_services.so
===> Checking for items in pkg-plist which are not in STAGEDIR
===> Error: Plist issues found.
*** Error code 1

Stop.
make: stopped in /usr/ports/databases/mysql57-server
====>> Error: check-plist failures detected

(In reply to Thomas Zander from comment #35)

The wording of the proposed UPDATING is not 100% correct, a more correct wording would be something like this:


Please keep in mind that the default location for my.cnf has changed from "/var/db/mysql/my.cnf" to "/usr/local/etc/mysql/my.cnf". Please merge your existing my.cnf with the new default one and move it to the new location. If you do not want to merge and move your existing my.cnf then you must set "mysql_optfile" in /etc/rc.conf to the location of your own my.cnf file.

Comment 38 Thomas Zander 2016-07-03 12:25:55 UTC

(In reply to Mahdi Mokhtari from comment #34)

Mokhi, are you going to address the plist issues from comment #36 ?
We should get this into the tree soonish, as it started blocking other PRs.
Thank you in advance.

Comment 39 Mahdi Mokhtari 2016-07-04 00:11:21 UTC

(In reply to Thomas Zander from comment #38)
Yes :)
It's fixed in my local tree.
I'm doing QA and update/regenerate patches today.
Sorry if it's lasted more than expected ;)

Comment 40 Mahdi Mokhtari 2016-07-04 00:44:24 UTC

Created attachment 172081 [details]
merging two patches about my.cnf place (updated+fixed pkg-plist issues)

Fixed pkg-list issue + portlint/poudriere check.
I guess it's ready now :D

Comment 41 Thomas Zander 2016-07-04 06:18:11 UTC

(In reply to Mahdi Mokhtari from comment #40)

OK, thanks for the latest patch set. USE_OPENSSL is deprecated now, but that can also be fixed in a future update.

However, we have to ensure the UPDATING entry is right. Based on Markus's proposal, I would add this to UPDATING:

20160704:
  AFFECTS: users of databases/mysql57-*
  AUTHOR: riggs@FreeBSD.org

  The default location for my.cnf has changed from "/var/db/mysql/my.cnf"
  to "/usr/local/etc/mysql/my.cnf". Existing my.cnf files must be merged
  manually with the new default and move it to the new location. To
  continue using the my.cnf file at the old location, set "mysql_optfile"
  in /etc/rc.conf to point to the location of the existing my.cnf file.

Let me know if this is OK. I have to emphasise the wording "has changed", because 
mysql57-server/pkg-message stills says "will be changed in the near future". So either UPDATING or pkg-message would be wrong.

(In reply to Thomas Zander from comment #41)

Your UPDATING entry is OK and correct.


mysql57-server/pkg-message and mysql57-server/files/mysql-server.in have to be corrected to be in par with UPDATING, but this can IMO also wait till next update.

Comment 43 Mahdi Mokhtari 2016-07-04 10:52:14 UTC

(In reply to Thomas Zander from comment #41)
(In reply to Markus Kohlmeyer from comment #42)
Hi.
It's Okay in my view :)
Thanks all guys :D, good job ;)

Comment 44 Thomas Zander 2016-07-04 13:46:22 UTC

Created attachment 172100 [details]
Updated patch, UPDATING and messages

Call me picky, but especially when introducing breaking changes I like to have to documentation in line with the actual behaviour.

Here is my proposal for the patch including UPDATING and the files containing messages in relation to this change.

Comment 45 Mahdi Mokhtari 2016-07-04 13:54:52 UTC

(In reply to Thomas Zander from comment #44)
About UPDATING entry, i agree it has better wording now :)
Did you changed Makefile or other patches behavior too ? or just UPDATING updated ?
Should i do QA (or at least poudriere) again ?

(In reply to Thomas Zander from comment #44)
I'm fine with your patch and the wording, thanks.

(In reply to Mahdi Mokhtari from comment #45)
He only changed the existing wording in mysql57-server/pkg-message and mysql57-server/files/mysql-server.in additional to your last patch.
Should pass QA without problems.

Comment 47 Mahdi Mokhtari 2016-07-04 14:03:53 UTC

(In reply to Markus Kohlmeyer from comment #46)
Okay thanks for clarifying things for me :)
I'm gonna approve+ it then.

Comment 48 commit-hook 2016-07-04 14:59:58 UTC

A commit references this bug:

Author: riggs
Date: Mon Jul  4 14:59:16 UTC 2016
New revision: 418033
URL: https://svnweb.freebsd.org/changeset/ports/418033

Log:
  Make my.cnf path correct according to hier(7), bump PORTREVISION

  PR:		209579
  Submitted by:	mokhi64@gmail.com
  Reviewed by:	rootservice@gmail.com

Changes:
  head/databases/mysql57-client/Makefile
  head/databases/mysql57-client/files/patch-mysys__ssl_my__default.cc
  head/databases/mysql57-server/Makefile
  head/databases/mysql57-server/files/mysql-server.in
  head/databases/mysql57-server/files/patch-mysys__ssl_my__default.cc
  head/databases/mysql57-server/pkg-message
  head/databases/mysql57-server/pkg-plist

Comment 49 commit-hook 2016-07-04 15:00:59 UTC

A commit references this bug:

Author: riggs
Date: Mon Jul  4 15:00:44 UTC 2016
New revision: 418034
URL: https://svnweb.freebsd.org/changeset/ports/418034

Log:
  Document change of default location for my.cnf for databases/mysql57-*

  PR:		209579
  Submitted by:	mokhi64@gmail.com
  Reviewed by:	rootservice@gmail.com

Changes:
  head/UPDATING

Comment 50 Mahdi Mokhtari 2016-07-04 15:22:43 UTC

(In reply to commit-hook from comment #49)
Thanks ;)
Good job all :)