> On April 25, 2013, 3:43 p.m., Edward Hades Toroshchin wrote: > > 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? > > Patrick von Reth wrote: > 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. > > > Edward Hades Toroshchin wrote: > > 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.
> 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 <mitch...@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 - Matěj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110187/#review31576 ----------------------------------------------------------- On April 25, 2013, 2:36 p.m., Patrick von Reth wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/110187/ > ----------------------------------------------------------- > > (Updated April 25, 2013, 2:36 p.m.) > > > Review request for Amarok. > > > Description > ------- > > 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". > > > Diffs > ----- > > src/core-impl/collections/db/sql/mysqlecollection/MySqlEmbeddedStorage.cpp > 0233498fdeb18ab51e709e9a78384fc37c47cb2a > > Diff: http://git.reviewboard.kde.org/r/110187/diff/ > > > Testing > ------- > > Only on windows > > > Thanks, > > Patrick von Reth > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
