> 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. > > Matěj Laitl wrote: > > 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
Edward, does that resolve your remarks? Would you object merging this? - 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
