News:

Simutrans Tools
Know our tools that can help you to create add-ons, install and customize Simutrans.

New directory structure

Started by prissi, February 11, 2022, 12:37:09 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

prissi

As suggested the simutrans directory structure does not really fit with modern OS any more. So we have there point to solve:

1) Many OS write protect the program directory. Thus additional data (like pak sets) and user data (save games) need /wantsto be saved elsewhere.
2) We want to have a portable version of simutrans.
3) In times of clouds having a cloud acces (google drive, ... ) to user data might be desireable too, so one coudl continue one game from Android to PC and back. Since paks do not change that often, they do not need to go there. Also on could edit a simuconf.tab on Android only this way.

1) Is easy to solve but just allowing mroe than one directory for pak sets and change pathes according. It has the disadvantage that each user duplicates their pak sets. However, compared to the three separate roblox installations (in AppData\Local\ since anybody has write access there) for my children taking up 40 GB on my laptop, this is neglble nowadays.
2) With some suitable subdirectory structure, this seems easy.
3) Might need some OS specific code ... ?

Roboron

Currently we have:

- data directory
- user data directory

While we need at least one more:

- install directory (because it can be write-only)
- pakset directory (because it must be writeable)
- user data directory

1. The pakset directory can still be equal to the data directory in systems where this directory is writeable (and portable installs). So we could check first if this directory is writeable, and otherwise use the user data directory for the pakset directory. Or is there a need to keep paksets and user data separate in this case?

2. In portable install, all would be the same directory as the binary is.

3. I'm not sure what you are suggesting here. That the user directory can be customized by the user to point it to a cloud directory?

4. A list of paths to search for install data (OS specific) might be also a good idea, specially since the "data dir" is usually assigned to the simutrans binary location, but this is not true in systems like linux or MacOS. In MacOS for example, the binary in the bundle would be in ".app/Content/MacOS" while the data should be in ".app/Content/Resources". So one could start with the binary location, but if it doesn't find the data there, check the other OS locations where it should be.

Matthew

As you work on this, could you please remember this use case?:

  • I have a Linux installation when I am root and a user and have installed Simutrans-Standard from Ubuntu
  • A child has his own user account
  • With the current setup, he can't add paksets because they're stored in a root-only directory
  • It would be nice if he could add paksets without needing a root password
At the moment the child in question is still too young to be trusted with his own user account, so he plays Simutrans under supervision. But when he's older I'd like him to be able to add paksets without me watching, especially since Simutrans is very safe compared to most PC usage (thank you to everyone who's made it that way!).

Obviously you can't control how Debian/Ubuntu package things.... but you can set defaults that they might never notice!

For example, Roboron's post says some things should be writeable.... but by who? Maybe that's clear to all of you because you have more experience, but it wasn't to me.
(Signature being tested) If you enjoy playing Simutrans, then you might also enjoy watching Japan Railway Journal
Available in English and simplified Chinese
如果您喜欢玩Simutrans的话,那么说不定就想看《日本铁路之旅》(英语也有简体中文字幕)。

prissi

#3
That is exactly the case we need to address. It means also that there are probably many copies of the pak set, if there is no gloabl writeable data directory, but that has been very common (see the Roblox example).

I also agree that we need three folders and should totally ignore the path of the executable (or just use it first search starting point)
1) Data that comes with (not touched at all by a normal user, like /usr/games/)
2) Pak set directory (usually not touched, if possible a global writable directory)
3) User content (pretty much what is now the user_dir, I think)

The default are compiled in simutrans with some overide by

  • OS dependent initialisation
  • environment variable(s)
  • simuconf.tab setting (although it is hard to change this in a write protected directory)
  • commandline switches (replacing use_workdir and set_dir)
  • GUI (at least for option 3, so the user can move this to a google drive/one dir/... cloud-synced directory)

There could be more than one at the start, which will be searched to find a valid one. They will be searched in some order. However, for later I suggest that only one path is allowed. (EDIT: Still paks/htmes/config/music can be in any of the three directories in a folder pak, music, themes, ... The orders will be searched and loaded as now, i.e. first base, then data, then user).

Now we should just come up with consistent names to use throughout and useful initialization. My suggestion
base_dir, pak_dir, user_dir to be sure catching all data_dir in the code.
And then name commandline switches, simuconf.tab entries, GUI etc. "set_base_dir" ...

Btw. Testing if a path is writeable is futile on Windows since the Programs path is user writeable, but all written content is discarded after the program finishes ...

Roboron

Quote from: Matthew on February 12, 2022, 01:13:24 AMRoboron's post says some things should be writeable.... but by who?

By the user who runs simutrans, for a start. Everything is writeable if you have the right permissions, but you shouldn't be running simutrans as sudo (?). But also for simutrans itself, because it could be sandboxed (MacOS, flatpak). Essentially these changes would solve both problems.

Quote from: prissi on February 12, 2022, 03:19:48 AMIt means also that there are probably many copies of the pak set, if there is no global writeable data directory, but that has been very common (see the Roblox example).

Steam does also install games in the user directory (per default, on linux at least) - so they are not shared with other users unless the installation directory is changed to a shared one.

Quote from: prissi on February 12, 2022, 03:19:48 AMshould totally ignore the path of the executable (or just use it first search starting point)

Checking it first would be necessary for portable installs.

Quote from: prissi on February 12, 2022, 03:19:48 AMsimuconf.tab setting (although it is hard to change this in a write protected directory)

I wonder if the simuconf.tab should really go into the base_dir. This file is clearly intended to be modified by the user, so it should go in the user_dir only. What is worse, from time to time I get invalid reports from users on Steam that modified the provided simuconf.tab, and then when Simutrans updates it gets overwrote, leading to complains about "X not working after last update" (when in reality it was the simuconf.tab being reset).

Quote from: prissi on February 12, 2022, 03:19:48 AMBtw. Testing if a path is writeable is futile on Windows since the Programs path is user writeable, but all written content is discarded after the program finishes ...

Ok, another approach: the pak_dir is unique and set for every system to a location we know will be writeable (by the pakset installer). The base_dir is then the only one we need to look for.

But Simutrans must also search for paksets in base_dir - because some paksets may be installed there by distributions or other actors.

PJMack

I would suggest only two directories be used, the installation directory and the user directory.  For paksets, simutrans would check in both directories to allow system admins to install read only paksets and still allow the user to install additional paksets.  In the event the same pakset is in both the user and system directories, the one in the user directory would be used.  Another game, Battle for Wesnoth, uses a similar system for its data files. Likewise for configuration data, a default configuration file would be placed in the installation directory which would only be used if the user does not have a copy of such file in their own directory.  The default configuration file would have instructions on where exactly to copy it to, and possibly a warning that it may become overwritten by a package manager.

prissi

We need three directories, since many data dirs (i.e. MacOS bundles, Debian distros) and read only. Moreover, the executable directory is often not writeable.

So I think it should go like this:

Test current directory => contains data => set to base_dir
=> if portable set in simuconf.tab, set folder "user/" to user_dir and pak_sir to "paks/" and finish

Repeat above test with executable directory (if not the same)

If base_dir no set => look for system specific base dir (/usr\games or enviroment etc.)

if user_dir not set => set user directory same as now

if base_dir contains writable (and non-shadowed) folder base_dir+"paks/ " => set to pak_dir
else use some system spefic pak_dir (environment, or folder pak in user_dir) or maybe just usr_dir+"paks/"

Roboron

Is there any justification to change pakset directory to "paks/" instead of the root as currently? Wouldn't that break the current way of deploying paksets?

Otherwise assigning the pak_dir to the user_dir seems sensible to me in most cases. Only edge case is if the user has set the user_dir to some cloud folder, it would be of little value to save paksets in the cloud.

Everything else looks good to me.

prissi

About the cloud dir: Yes and no. On Android this would be the only way to manipulate paks ... On the other hand, if we use a folder like AppData/local/simutrans then one would need also a windows uninstaller for simutrans.

I think paks under a subdir would keep the structure much cleaner in the user dirs, since there is also an addon folder with a similar structure. Also having on two directories pointing to the same locations does not sound like a good idea.

Roboron

Okay, if you wanted to change the structure for better organization it is indeed the best opportunity to do so.

prissi

r10520 contains the first start. @Roboron, I think you want to add an isntallpath to your function.

Also by default we have SIMUTRANS_???DIR with BASE INSTALL and USER to redirect the paths.

The install path is currently useless, but that will change quickly.

Roboron

Quote from: prissi on March 02, 2022, 02:45:32 PM@Roboron, I think you want to add an installpath to your function.

What is "my function"? I don't know I had one (?)

Reviewed your patch and corrected some typos and other minor corrections. Also updated paths in the fluidsynth files that made the CI to fail.

But dr_query_installdir() should not return "/usr/share/simutrans" since this is not global writeable. This is the base_dir (that can contain paksets but only if installed by the system), and not the install_dir (that I'm afraid can only be in the user home). I guess we need another function (dr_query_basedir) that check for a list of paths an return the one with a valid installation (current dir => other system-dependent dirs, because the binary is not always in the same place that the base install (as in the MacOS bundle or the linux /bin vs /share).

prissi

Ok, seems I googled old answers. I checked on a current Debian, and indeed, neither /var nor /usr/ nor /srv is writeable to a normal user anymore, only /tmp which is not a good place for permanent data. This is even worse than windows. Especially since a directory with no execution rights should not be a problem with secure linux. (I do not want to run the simutrans with root rights to be able to download content. This is broken on many levels. Sigh)

An interesting way out (apart from throwing everything in the user directories), would be the creation of a simutrans group, and creating a group writeable folder under /var/lib/simutrans during installation. That would neet root access only once. simutrans could ask the user to start a script creating /var/lib/simutrans and the group if it does not exist and otherwise use "~/simutrans/paksets" for install directory. Simutrans may the use setgid() to change to access this and back.

The challenge will be to get such script executed at least once (and writing this to get simutrans the needed informations). Maybe it could be integrated in the pakdownloader. However, I think my outdated Linux knowhow has reached its limit here.

On the basedir:

The mac bundle has code the find its base dir, if I did not broke it.

On Linux, using the system values for the self installed simutrans would mean the correct base installation might not been found when was forgotten to be downloaded. Therefore, I would suggest using the current dir and giving up if no other hints are set.

We could make it more easy for distribution by using a define (-DSIMUTRANS_BASEDIR="/usr/games/share/") on compile time ...

Yona-TYT

Supertux save the data here:

prissi

Yes, but this is local user data, while paksets should be global.

Roboron

Quote from: prissi on March 03, 2022, 06:18:57 AMThe challenge will be to get such script executed at least once (and writing this to get simutrans the needed informations). Maybe it could be integrated in the pakdownloader. However, I think my outdated Linux knowhow has reached its limit here.

We should use the user home for now. That is an improvement already. We can go back to this when we are finished.

Quote from: prissi on March 03, 2022, 06:18:57 AM
On Linux, using the system values for the self installed simutrans would mean the correct base installation might not been found when was forgotten to be downloaded. Therefore, I would suggest using the current dir and giving up if no other hints are set.

On linux we can use a solution similar to the macOS one: Instead of checking if we are running inside and application bundle, we can check if we are running in a "bin/" directory, which would mean that simutrans has been installed by the system. Assuming that the installation prefix is common (which will be true basically for all installations not explicitly modified), we can check for the base_dir relative to our location (replacing "bin/" with "share/simutrans"). That should work most of the time, including Debian/Ubuntu/Arch and flatpak installations. In r10524.

prissi

Thanks. I think we need to make sure that the local directory is not already a base dir even if the user has it in a bin folder, before trying other locations. See 10525

Roboron

The r10524 solution did not work because Simutrans can be started from *any* working directory, not only the binary directory. Luckily I found an equivalent solution, which consists on first looking for the binary location using readlink + "/proc/self/exe", and then proceed as before. This seems to work on a flatpak environment too, which was my biggest concern. In r10527.

prissi

argv[0] is filled with this (and other methods for all platforms) already in simsys.cc. So no need to duplicate it.

Also using the installation dir for search for base dir makes some sense before finally giving up. See 10528

Roboron

tstrncpy(testpath, argv[0], lengthof(testpath));

The value of testpath after this assignment is "/simutrans", which is not the full path (nor any useful path).

prissi

simsys.cc line 1160 reads /proc/self/exe already. I get correct values on Debian in argv[0] in simmain, set by simsys.cc. What value has your argv[0] inside simmian?

Roboron

Sorry, it was not testpath the one with "/simutrans" value, but the "c" variable later derived from it. testpath value seems to be the full path, actually.

prissi

Ah, yes, but I think I fixed it this morning.

Roboron

#23
Mmm, no, you didn't. [EDIT: misunderstandings about the c variable]

EDIT: Okay, I think I fixed it in r10532, by adding an additional assignment for c after empty it. Also added much needed assignments for found_basedir

prissi

c is not empty, c is a pointer to testpath, not std::string. (c-3) points to bin (or wahtever is three letters back). *c points to the rightmost path separator, then going back three further. Ergo bin You are now going back two path separators, pointing to usr

PJMack

Security is indeed the reason that there are not shared user writable folders in most Linux distributions.  (The tmp folder has the caveat that only the user that wrote a file can read it.)   The only way to install a pakset system-wide is by the root user (or sudo) and as prissi mentioned, running simutrans as root is not an option.  The most common technique I have seen for software in a similar situation is to have a separate package for each sanctioned addon (pakset in the case of simutrans) which are installed by the package manager alongside the installation directory, as well as an addons folder for each user to allow them for to add their own.  I highly recommend we follow this convention.

ceeac

Bug: Running simutrans from the simutrans/ directory in a fresh checkout does no longer work, i.e.

make && cd simutrans && ../sim -use_workdir -objects pak -debug 2

now returns

dr_fatal_notify: ERROR: No fonts found!


EDIT: set_pakdir does a dr_chdir() but there is no dr_chdir() back to the original location.

prissi

Sorry, the pak_dir logic is not yet finished. This is still under construction.

As to security: The biggest risk is in front of the keyboard. It is a higher security risk to give my seven year old sudo priviledges to download and install paksets than having a data only folder from which nothing can be executed.

Since security allows me to make a file in my folder accessible to all with chmod 777 xxx, the security by denying a global non-executable data directory is rather negative in my eyes and just to show we can do. Older Unix had writable /usr/lib or /var/lib (rather decades ago, I admid).


Roboron

Quote from: PJMack on March 05, 2022, 12:03:40 AM
Security is indeed the reason that there are not shared user writable folders in most Linux distributions.  (The tmp folder has the caveat that only the user that wrote a file can read it.)   The only way to install a pakset system-wide is by the root user (or sudo) and as prissi mentioned, running simutrans as root is not an option.  The most common technique I have seen for software in a similar situation is to have a separate package for each sanctioned addon (pakset in the case of simutrans) which are installed by the package manager alongside the installation directory, as well as an addons folder for each user to allow them for to add their own.  I highly recommend we follow this convention.

The first option is already the case with Simutrans. We aim to add the second option with these changes. I personally think that using the user directory would be good enough, but using a global directory has its advantage: it avoids duplication of paksets.

prissi

r10535 should hopefully correctly use pak_dir and pak_name and should even be able to handle pakset in base_dir, isntall_dir and user_dir/simutrans

PJMack

Quote from: Roboron on March 05, 2022, 10:06:12 AMThe first option is already the case with Simutrans. We aim to add the second option with these changes. I personally think that using the user directory would be good enough, but using a global directory has its advantage: it avoids duplication of paksets.
I was under the impression that the intent was to have simutrans create a single shared user writable directory for paksets.  If this was not the case, then I apologize for the confusion. 

Roboron

Quote from: prissi on March 05, 2022, 01:39:14 PM
r10535 should hopefully correctly use pak_dir and pak_name and should even be able to handle pakset in base_dir, isntall_dir and user_dir/simutrans

What's the purpose of this change?

@@ -444,9 +444,10 @@ char const *dr_query_installdir()
tstrncpy(buffer,SDL_GetPrefPath("Simutrans Team","simutrans"),lengthof(buffer));
#else
if( getenv("XDG_DATA_HOME") == NULL ) {
- sprintf(buffer, "%s/simutrans", getenv("HOME"));
- } else {
- sprintf(buffer, "%s/simutrans", getenv("XDG_DATA_HOME"));
+ sprintf(buffer, "%s/simutrans/simutrans", getenv("HOME"));
+ }
+ else {
+ sprintf(buffer, "%s/simutrans/simutrans", getenv("XDG_DATA_HOME"));
}
#endif

prissi

The installer is geared that paks are inside a directory simutrans. Hence, it would be the easiest to have as installable directory one called "simutrans". Since some paks overwrite themes, it could clash with user themese, if the paks would go directly in the userdir.

Roboron

Wouldn't it better to accommodate the installer to install paksets in a "paksets" subdirectory? That's... more descriptive. Having a "simutrans" directory inside another "simutrans" directory seems weird to me (specially in the case the user dir is set to be elsewhere when this will be the only item).

prissi

It is true, but I am not sure how to do this with the scripted installer without first extracting to another directory containing a simutrans folder. The builtin installer in Simutrans itself has no restrictions.