<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/110187/">http://git.reviewboard.kde.org/r/110187/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 25th, 2013, 3:43 p.m. UTC, <b>Edward Hades Toroshchin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for the fix. However, this is the approach, we've used originally, and it didn't prove very reliable. I think we even went back and forth more than once.
So you should really have a look in the history of the mysql collection, to see if and how this is different from the way it's been done before (and why it isn't done that way anymore).
> This fixes an issue where amarok is writing the database to "C:\Program Files (x86)\Amarok\data\amarok".
> This issue prevents Amarok from running correctly, because regarding to the rights of the useraccount the directory can be not writeable.
It's not only that, it could also lead to problems if several users tried to use Amarok simultaneously.
I suggest you check, where does this path come from. Why is storageLocation empty?</pre>
</blockquote>
<p>On April 25th, 2013, 5:46 p.m. UTC, <b>Patrick von Reth</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Storage location is not empty but either mysql isn't using MYSQL_HOME or unable to read the file.
Why should it be a problem if several user are running amarok, what would be the difference to the my.cnf approach.
</pre>
</blockquote>
<p>On April 26th, 2013, 11:08 a.m. UTC, <b>Edward Hades Toroshchin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> Why should it be a problem if several user are running amarok, what would be the difference to the my.cnf approach.
No, I was talking about the my.cnf approach, since the my.cnf file would be in the same place for different users.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> However, this is the approach, we've used originally, and it didn't prove very reliable. I think we even went back and forth more than once. So you should really have a look in the history of the mysql collection, to see if and how this is different from the way it's been done before (and why it isn't done that way anymore).
This was my initial comment too, please see my IRC discussion with Patrick that led to this patch.
TL;DR: the last transition to my.cnf was in Jeff's commit that simply stated "use MYSQL_HOME and generate my.cnf on the fly instead of hardcoding. Will be necessary later." - however we were unable to find later commits that would made use of my.cnf necessary. (but we haven't been looking real hard)
When I ignore the past (which we should not), passing arguments directly looks more reliable than my.cnf + env vars.
-------------------------------------------
[Wednesday 24 of April 2013] [15:27:35] <TheOneRing> strohel: is there a reason why amarok is using the my.cnf instead of passing the arguments directly?
[Wednesday 24 of April 2013] [15:33:14] <strohel> TheOneRing: I thought there were come problems with it.
[Wednesday 24 of April 2013] [15:33:21] <strohel> TheOneRing: lemme dig it
[Wednesday 24 of April 2013] [15:39:01] <strohel> TheOneRing: commit 7abdb3421: Jeff Mitchell <mitchell@kde.org> Mon Jul 19 2010 Work around longstanding mysqle-loads-innodb issues with changing to loose-innodb=0; but more importantly, use MYSQL_HOME and generate my.cnf on the fly instead of hardcoding. Will be necessary later.
[Wednesday 24 of April 2013] [15:39:02] <kgbot> 4Jeff Mitchell master * [14http://quickgit.kde.org/?p=amarok.git&a=commit&h=7abdb34] Work around longstanding mysqle-loads-innodb issues with changing to loose-innodb=0; but more importantly, use MYSQL_HOME and generate my.cnf on the fly instead of hardcoding. Will be necessary later.
[Wednesday 24 of April 2013] [15:39:20] <strohel> Wow, kgbot++
[Wednesday 24 of April 2013] [15:39:35] <strohel> TheOneRing: But I don't see the "will be necessary later"
[Wednesday 24 of April 2013] [15:40:23] <strohel> TheOneRing: If all the options are passable as arguments, feel free to change it to pass the args directly (but run through reviewboard pls.)
[Wednesday 24 of April 2013] [15:40:27] <TheOneRing> so we could pass the stuff as arguments as far as I understand the documentation
[Wednesday 24 of April 2013] [15:40:37] <TheOneRing> kk
[Wednesday 24 of April 2013] [15:41:30] <strohel> TheOneRing: you can use revert of the mentioned patch as basis, but I'd suggest a cleaner approach. (i.e. not "char *" variables, just QStrings and their toLocal8bit().data() when you need char *)
[Wednesday 24 of April 2013] [15:41:54] <TheOneRing> yes
[Wednesday 24 of April 2013] [15:42:03] <TheOneRing> local 8bit or latin1?
[Wednesday 24 of April 2013] [15:42:59] <strohel> TheOneRing: I think the local8bit is more correct - mysql should assume the that the parameters are specified in currently selected locale's charset - and that's exactly what toLocal8bit does.
[Wednesday 24 of April 2013] [15:43:36] <TheOneRing> kk
[Wednesday 24 of April 2013] [15:43:44] <strohel> TheOneRing: (at least on Linux; do check with home directory containing accented and special characters on Win, I'll check on Linux once you submit the patch)
[Wednesday 24 of April 2013] [15:45:35] <strohel> TheOneRing: do mysql methods take ownerwhip of the passed char * arguments? If so, you'd have to wrap them with qstrdup().
[Wednesday 24 of April 2013] [15:46:18] <TheOneRing> . mysql_library_init() makes a copy of the arguments so it is safe to destroy argv or groups after the call.
[Wednesday 24 of April 2013] [15:46:28] <strohel> TheOneRing: good
[Wednesday 24 of April 2013] [15:47:20] <strohel> TheOneRing: also note that QString::toLocal8bit() creates temporary QByteArray - it's data() pointer is only valid for lifetime of the QByteArray.
[Wednesday 24 of April 2013] [15:47:45] <strohel> TheOneRing: i.e. "char *something = QString( "Hi" ).toLocal8Bit().data()" is wrong
[Wednesday 24 of April 2013] [15:51:53] <TheOneRing> kk
[Wednesday 24 of April 2013] [15:52:57] <TheOneRing> strohel: could you have a look on "mysqld --verbose --help > mysql-help.txt" I can find every setting defined in the my.cnf but the loose-innodb
[Wednesday 24 of April 2013] [15:53:22] <TheOneRing> but a lot of settings regarding innodb
[Wednesday 24 of April 2013] [15:53:34] <strohel> TheOneRing: hmm, that may actually be aproblem.
[Wednesday 24 of April 2013] [15:54:26] <strohel> TheOneRing: perhaps you can find another option that disables loading od the innodb engine?
[Wednesday 24 of April 2013] [15:54:26] <kgbot> 4Matěj Laitl master * [14http://quickgit.kde.org/?p=amarok.git&a=commit&h=0f59c66] IpodCopyTracksJob: even better error messages
[Wednesday 24 of April 2013] [15:55:19] <strohel> TheOneRing: --ignore-builtin-innodb ?
[Wednesday 24 of April 2013] [15:55:24] <TheOneRing> http://dev.mysql.com/doc/refman/5.6/en/innodb-turning-off.html
[Wednesday 24 of April 2013] [15:57:52] <TheOneRing> ok it looks like amarok offers the possebility to use a handcrafted my.cnf ...
[Wednesday 24 of April 2013] [15:58:17] <strohel> TheOneRing: does it?
[Wednesday 24 of April 2013] [15:59:48] <strohel> TheOneRing: I've found that 7abdb3421 in effect just changed --loose-skip-innodb to loose-innodb -- si it shouldn't be a problem as there seem to be other ways to disable it.
[Wednesday 24 of April 2013] [15:59:49] <kgbot> 4Jeff Mitchell master * [14http://quickgit.kde.org/?p=amarok.git&a=commit&h=7abdb34] Work around longstanding mysqle-loads-innodb issues with changing to loose-innodb=0; but more importantly, use MYSQL_HOME and generate my.cnf on the fly instead of hardcoding. Will be necessary later.
[Wednesday 24 of April 2013] [16:00:18] <TheOneRing> kk
[Wednesday 24 of April 2013] [16:00:22] <TheOneRing> http://quickgit.kde.org/?p=amarok.git&a=blob&h=0233498fdeb18ab51e709e9a78384fc37c47cb2a&hb=e033f7c07a476a9641a8c04ce73b57ecb8209df3&f=src%2Fcore-impl%2Fcollections%2Fdb%2Fsql%2Fmysqlecollection%2FMySqlEmbeddedStorage.cpp line 54
[Wednesday 24 of April 2013] [16:00:22] <kgbot> 4Patrick von Reth master * [14http://quickgit.kde.org/?p=amarok.git&a=commit&h=e033f7c] updated scripts for 2.7.0 relesae
[Wednesday 24 of April 2013] [16:01:39] <strohel> TheOneRing: Hmm, I'd just ditch the hidden option. Please do it - as the patch will do though reviewboard other devs will have a change to object.
[Wednesday 24 of April 2013] [16:01:54] <strohel> *chance
[Wednesday 24 of April 2013] [16:28:40] <TheOneRing> strohel: not yet ready but at least it is working :D
[Wednesday 24 of April 2013] [16:29:06] <strohel> TheOneRing: good, hope it won't break Linux.
[Wednesday 24 of April 2013] [16:34:36] <TheOneRing> strohel: does http://paste.kde.org/731414/ look ok?
[Wednesday 24 of April 2013] [16:34:59] <TheOneRing> ah
[Wednesday 24 of April 2013] [16:35:09] <TheOneRing> I betetr remove the old support completely
[Wednesday 24 of April 2013] [16:35:34] <TheOneRing> because I would need to call setenv again in this approach
[Wednesday 24 of April 2013] [16:36:01] <strohel> TheOneRing: yes, do
[Wednesday 24 of April 2013] [16:36:14] <strohel> TheOneRing: also please mind Amarok coding style - spaces around args
[Wednesday 24 of April 2013] [16:36:43] <strohel> TheOneRing: qstrdup() is actually not needed for string literals ("Amarok")
[Wednesday 24 of April 2013] [16:37:30] <strohel> TheOneRing: Or: you've said mysql funcs copy the arguments - qstrdub is actually *never* needed then.
[Wednesday 24 of April 2013] [16:37:47] <TheOneRing> hm yes but the delete would fail?
[Wednesday 24 of April 2013] [16:39:06] <strohel> TheOneRing: oh yes, but the fact is that with proper use of QByteArrays you wouldn't have to call the delete at all.
[Wednesday 24 of April 2013] [16:39:49] <strohel> TheOneRing: I'd just construct the args as QList<QByteArray> and then create a char **mysql_argv; array of it in a simple foreach loop.
[Wednesday 24 of April 2013] [16:40:41] <strohel> TheOneRing: BTW, why don't you use git? ;)
*** Logfile started
*** on Wed Apr 24 16:53:02 2013
[Wednesday 24 of April 2013] [16:53:02] Topic The channel topic is "commitfilter dead, use projects.kde.org -> Follow this project (log in) | 2.8 & 3.0 Roadmap: http://bit.ly/TO7PbI | Component Bugs: bit.ly/OBZ8c7 | bored? http://tinyurl.com/65jf64 | calendar: http://tinyurl.com/qj63r |important links: http://tinyurl.com/6krsuy |Bugs: http://tinyurl.com/allamarokbugs |Continuous integration: http://build.kde.org".
[Wednesday 24 of April 2013] [16:53:02] Topic The topic was set by strohel!~strohel@193.86.155.71 on 8. 3. 2013 17:57.
[Wednesday 24 of April 2013] [16:53:04] Mode Channel modes: no messages from outside, secret
[Wednesday 24 of April 2013] [16:53:04] Created This channel was created on 26. 11. 2006 07:42.
[Wednesday 24 of April 2013] [16:53:30] <strohel> TheOneRing: sorry, net connection problems. I think it failed to send:
[Wednesday 24 of April 2013] [16:53:37] <strohel> heOneRing: I'd just construct the args as QList<QByteArray> and then create a char **mysql_argv; array of it in a simple foreach loop.
[Wednesday 24 of April 2013] [16:53:44] <strohel> TheOneRing: BTW, why don't you use git? ;)
[Wednesday 24 of April 2013] [16:55:10] <TheOneRing> because I curently only have a usable build of the 2.7.0 tag and dont want to rebuild everything^^
[Wednesday 24 of April 2013] [17:04:41] <TheOneRing> strohel: http://paste.kde.org/731438/
[Wednesday 24 of April 2013] [17:09:05] <TheOneRing> I will open a review request tomorrow or later this evening
[Wednesday 24 of April 2013] [17:24:32] <strohel> TheOneRing: looks good on first glance</pre>
<br />
<p>- Matěj</p>
<br />
<p>On April 25th, 2013, 2:36 p.m. UTC, Patrick von Reth wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok.</div>
<div>By Patrick von Reth.</div>
<p style="color: grey;"><i>Updated April 25, 2013, 2:36 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Don't communicate with mysql by env vars and autogenerated files
Instead of generating the my.cnf every time amarok starts and to pass
the location of this file by setting an environment variable directly
pass the settings as arguments to mysql. As this is probably a better
approach and the only one working on windows.
This fixes an issue where amarok is writing the database to "C:\Program Files (x86)\Amarok\data\amarok".
This issue prevents Amarok from running correctly, because regarding to the rights of the useraccount the directory can be not writeable.
The commands used are taken from the output of "mysqld --verbose --help". </pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Only on windows</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/core-impl/collections/db/sql/mysqlecollection/MySqlEmbeddedStorage.cpp <span style="color: grey">(0233498fdeb18ab51e709e9a78384fc37c47cb2a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110187/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>