USE_HOME_FOR_STATE_DIR option added #490
Conversation
ghost
mentioned this pull request
There was a problem hiding this comment.
looks good,
so cmake option can have the same identifier used as cxxflag? I mean, it wont conflict or cause confusion right?
now looking at GetStateDir() I guess the generated code will have 2 returns when that flag is used and it didnt cause a compilation warning for all platforms by travis-ci. As at travis-ci none uses such flag, it didnt errored!
it is fun to learn a bit more about CMakeLists :)
|
If I understand you correctly, everything is fine. 😎 In addition, the CMakeLists.txt has other options, such as Or you suggest making changes to the .travis.yml file to test the build with this option? |
|
@AquariusPower @fejoa @andrewtweber |
Main/Source/game.cpp
Outdated
| @@ -5191,6 +5191,9 @@ festring game::GetDataDir() | |||
|
|
|||
| festring game::GetStateDir() | |||
| { | |||
| #ifdef FORCE_HOME_AS_STATE_DIR | |||
| return GetHomeDir()+"/.ivan/"; | |||
There was a problem hiding this comment.
| return GetHomeDir()+"/.ivan/"; | |
| return GetHomeDir(); |
Please check the definition of game::GetHomeDir(). "/.ivan" is already included there.
There was a problem hiding this comment.
festring game::GetHomeDir()
{
#ifdef UNIX
festring Dir;
Dir << getenv("HOME");
#ifdef MAC_APP
Dir << "/Library/Application Support/IVAN/";
#else
Dir << "/.ivan/";
#endif
#ifdef DBGMSG
dbgmsg::SetDebugLogPath(Dir.CStr());
#endif
return Dir;
#endif
I think that /.ivan/ will be added only in case of non-UNIX system (i.e. Windows). While there is an option for UNIX systems. I roughly explained the situation here. In my Nix package everything is located inside ~/.ivan/ as expected. There is no additional nested /.ivan directory. It is already available on the unstable NixOS channel.You can check (for Linux). 👽
There was a problem hiding this comment.
Currently, non-UNIX system for IVAN is only Windows. And the executable for Windows is expected to run inside the game folder, so in that case Dir is an empty string meaning the current working directory, then /.ivan/ is inappropriate.
Where is your high score file? Shouldn't it be in ~/.ivan/.ivan now?
There was a problem hiding this comment.
I'm sorry, you were right. Fixed. 😐
There was a problem hiding this comment.
GetHomeDir is also a confusing function name on unix systems. :p The files reside there also store some "states" (config, saves, debug info). Maybe we should merge GetHomeDir() and GetStateDir() because it is so easy to cheat and there is currently no safe method to share high score files and bone files among multiple users and limit file access. (Then I will propose removing -DLOCAL_STATE_DIR="${CMAKE_INSTALL_FULL_LOCALSTATEDIR}/ivan"), which is the only hard-coded path for dynamic game data now. Another pull request will do.)
There was a problem hiding this comment.
@iology Thanks for merging! 🎉
I think I agree with you. In addition, a new class of operating systems that take a new approach to managing system configuration and packages, such as NixOS, GuixSD, Fedora Silverblue, do not allow applications to write data to random paths.
Although it was an old tradition to have a shared highscores and bones files in old roguelikes, but the new reality changes everything.
The new pull request is here!
CMakeLists.txt
Outdated
| @@ -13,10 +13,14 @@ set(CMAKE_CXX_FLAGS "-std=c++11 ${CMAKE_CXX_FLAGS}") | |||
| add_definitions(-DIVAN_VERSION="${PROJECT_VERSION}" -DUSE_SDL) | |||
|
|
|||
| option(BUILD_MAC_APP "Build standalone application for MacOS" OFF) | |||
| option(FORCE_HOME_AS_STATE_DIR "Statedir will be /.ivan/ in current user's homedir" OFF) | |||
There was a problem hiding this comment.
USE_HOME_FOR_STATE_DIR is a better name.
There was a problem hiding this comment.
As you wish. I changed that. 😎
Besides, I have reverted the changes to the .travis.yml file (I wasn't sure they were okay).
ghost
changed the title
|
|
||
| if(UNIX) | ||
| add_definitions(-DUNIX) | ||
| include(GNUInstallDirs) | ||
| if(USE_HOME_FOR_STATE_DIR) |
There was a problem hiding this comment.
The LOCAL_STATE_DIR define below should be enclosed in if(USE_HOME_FOR_STATE_DIR).
There was a problem hiding this comment.
I'm afraid that this PR is already merged. But you can take a look at the new one.
| @@ -5191,6 +5191,9 @@ festring game::GetDataDir() | |||
|
|
|||
| festring game::GetStateDir() | |||
| { | |||
| #ifdef USE_HOME_FOR_STATE_DIR | |||
There was a problem hiding this comment.
It would have been cleaner to put this in the #else of the #ifdef MAC_APP, as it's only relevant for non-OSX Unixes, as OSX and Windows already default to GetHomeDir().
USE_HOME_FOR_STATE_DIRoption was added to CMakeLists.txt.Fully consistent with my pull request to nixpkgs upstream.