MesserLab / SLiM

SLiM is a genetically explicit forward simulation software package for population genetics and evolutionary biology. It is highly flexible, with a built-in scripting language, and has a cross-platform graphical modeling environment called SLiMgui.
https://messerlab.org/slim/
GNU General Public License v3.0
164 stars 33 forks source link

LogFile maybe tries to write to wrong path on Windows #452

Closed petrelharp closed 4 months ago

petrelharp commented 4 months ago

Okay, I don't have windows, but in trying to get Windows CI tests for stdpopsim working, I came across this error:

E           #ERROR (Eidos_WriteToFile): could not write to file at path D:\a\stdpopsim\stdpopsim/C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\test_logfile0\slim.log.
E           
E           Error on script line 574, character 36:
E           
E               defineConstant("log", community.createLogFile("C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\test_logfile0\\slim.log", logInterval=NULL));
E                                               ^^^^^^^^^^^^^

That looks pretty suspicious: D:\a\stdpopsim\stdpopsim/C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\test_logfile0\slim.log looks like two well-formed Windows paths stuck together with a /, as if SLiM thought that C:\... was a relative path and tried to make it an absolute path by prepending the current directory.

Indeed, over in the log file code, we see this line:

    resolved_file_path_ = Eidos_AbsolutePath(user_file_path_);

and it is Eidos_AbsolutePath( ) that will stick together paths with a / (see #451). Meanwhile, the function Eidos_AbsolutePath( ) is not used anywhere else in SLiM's code; other places that files are written seem to use Eidos_ResolvedPath( ):

        outfile_path = Eidos_ResolvedPath(filePath_value->StringAtIndex_NOCAST(0, nullptr));

I'm suspecting that Eidos_AbsolutePath in the logfile code was supposed to be Eidos_ResolvedPath?

petrelharp commented 4 months ago

Reading the code for Eidos_AbsolutePath I can't see why it would have thought that C:\Users\... wasn't an absolute path, however - perhaps there's an error with escaping backslashes? But that is maybe a different issue.

bhaller commented 4 months ago

The use of Eidos_AbsolutePath() is correct. The goal here (unlike most other places in the code) is to get an absolute path even if a relative path is supplied. This is because relative paths are resolved by the operating system relative to the current working directory (CWD), and LogFile needs to stay pointed at the same destination even if the current working directory changes later on. I.e., if "bar.csv" is given to LogFile as the path to write to, it needs to stay pointed at the same directory even if the CWD changes later on; you don't want to be spraying "bar.csv" files around in different directories as the CWD gets changed during the run of the model with calls to setwd(). That is not a concern in most other places in the code, because most other places just write once to the path given; LogFile is one of just a few places that hangs on to a path log-term and thus needs to resolve that path to an absolute path. (See also Eidos_ExecuteFunction_flushFile() and Eidos_ExecuteFunction_writeFile(), because of buffering of writes in some cases.) Note that Eidos_AbsolutePath() calls Eidos_ResolvedPath(), as practically the first thing it does; it just then performs the additional step of making the path absolute using the CWD (and does that wrong on Windows when \ is used, as discussed in #451).

So, no bug here. The bug is in #451, and the workaround until that issue is fixed, as discussed over there, is to just use / instead of \ on Windows, as on other platforms; just say no to \.