Bug 209579
| Summary: | databases/mysql57-server, databases/mysql57-client: Make my.cnf path correct according to hier(7) and last upgrade | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Mahdi Mokhtari <mmokhi> | ||||||||||||||||||
| Component: | Individual Port(s) | Assignee: | Thomas Zander <riggs> | ||||||||||||||||||
| Status: | Closed FIXED | ||||||||||||||||||||
| Severity: | Affects Some People | CC: | mmokhi, riggs, rootservice | ||||||||||||||||||
| Priority: | --- | Keywords: | patch, patch-ready | ||||||||||||||||||
| Version: | Latest | ||||||||||||||||||||
| Hardware: | Any | ||||||||||||||||||||
| OS: | Any | ||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 210758 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Mahdi Mokhtari
2016-05-17 14:00:31 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
(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 (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 (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.
(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 ;) (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 ? (In reply to Mahdi Mokhtari from comment #14) never mind :D ignore my previous comment ;) Both client and server parts are needed ;) 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
(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
(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. (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) Created attachment 170435 [details]
merging two patches about my.cnf place
QA previously done Okay.
Patch is ready to commit.
Thanks Markus ;) Please don't forget the UPDATING entry ;) (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 on attachment 170441 [details]
UPDATING entry
Thanks for rewording
Markus ;)
(In reply to Mahdi Mokhtari from comment #28) Ready to be committed in IMO :) DeAssigning from myself so that a committer comes and takes it faster :) Is this up to date since mysql has been updated to new upstream versions? (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) 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
(In reply to Thomas Zander from comment #31) @Thomas Zander Done :) Both patches are now updated and ready to commit in ;) 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 :) 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. (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. (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 ;) 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
(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. (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 ;) 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.
(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. (In reply to Markus Kohlmeyer from comment #46) Okay thanks for clarifying things for me :) I'm gonna approve+ it then. 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 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 (In reply to commit-hook from comment #49) Thanks ;) Good job all :) |