On Fri, Mar 21, 2014 at 10:47 AM, Benjamin Piwowarski <bpiwo...@lyx.org>wrote:
> On 21 Mar 2014 at 10:07:21 , Vincent van Ravesteijn (v...@lyx.org) wrote: > > From: Benjamin Piwowarski <bpiwo...@lyx.org> >> Date: Fri, 14 Mar 2014 10:48:40 +0000 (+0100) >> Subject: Fix problem with python and change of PATH >> X-Git-Url: >> http://git.lyx.org/?p=lyx.git;a=commitdiff_plain;h=49b943e8458c6d23a7db29033e92601c7404fff5 >> >> Fix problem with python and change of PATH >> >> - waits that lyxrc has been read before finding python >> - when the PATH changes, resets the value >> --- >> >> << "\tbinary_dir " << binary_dir().absFileName() << '\n' >> @@ -156,6 +151,19 @@ Package::Package(string const & command_line_arg0, >> << "</package>\n"); >> } >> >> +std::string const & Package::configure_command() const >> +{ >> + if (configure_command_.empty()) { >> + std::string &command = const_cast<std::string&>(configure_command_); >> + FileName const configure_script(addName(system_support().absFileName(), >> "configure.py")); >> + command = os::python() + ' ' + >> + quoteName(configure_script.toFilesystemEncoding()) + >> + with_version_suffix() + " --binary-dir=" + >> + quoteName(FileName(binary_dir().absFileName()).toFilesystemEncoding()); >> + } >> + return configure_command_; >> +} >> + > > > Either deconsitfy this function, or make configure_command_ a mutable > member. In any case, do not use const_cast<>. Moreover, try not to use > non-const references if not explicitly needed. Especially not if it points > to a variable that is used in the same function under a different name. > > OK, I check the docs about development style but found no clue about what > is best. I would tend to use a mutable since it is not supposed to change > the overall outcome of the method. Or maybe better, this value can be > recomputed each time (they are not that many calls to configure_command) > I don't know why this value needs to be cached. It would be needed if one of the components is a costly operation (like searching for the python executable), but I don't see one immediately. By the way, there is much more wrong with the Package class. Most notably: FileName & document_dir() const { return document_dir_; } A const function returning a non-const reference to a mutable member ????? > > > >> >> >> -string const python() >> +string const python(bool reset) >> { >> // Check whether the first python in PATH is the right one. >> static string command = python2("python -tt"); >> + if (reset) { >> + command = python2("python -tt"); >> + } >> >> if (command.empty()) { >> // It was not, so check whether we can find it elsewhere in > > > I don't know whether I completely like this. Maybe we should have Package > store the value of the python interpreter, and make an explicit function: > findPython to update the member. > > I think the overall python finding should be changed, but this was to > provide a reasonable fix to the problem of the PATH changing with a > relative path for python > > Yes, I understand, but I don't like the concept of a static caching variable, while we have a Package class taking care of this. Vincent
