Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #7311: External configuration file does not change directories #7339

Closed
wants to merge 1 commit into from

Conversation

@Berbe
Copy link
Contributor

@Berbe Berbe commented Mar 7, 2019

_personal_dir gets replaced by config_dir outside of the XDG triggers if a configuration file is specified.
This makes directories path computation fail to find content sitting in normal game arborescence, such as the autoload directory.

This prosposal aims at preventing this behaviour, by not touching the arborescence when a configuration file is specified.
Discovered & loaded files should thus be more consistent across executions.

Any drawback?

@Berbe Berbe force-pushed the personal-dir branch from 837be87 to a5229e5 Mar 7, 2019
@Berbe Berbe changed the title Fix #7311: External configuration file does not change arborescence Fix #7311: External configuration file does not change directories Mar 7, 2019
@Berbe
Copy link
Contributor Author

@Berbe Berbe commented Mar 7, 2019

At least, this makes regression tests pass without patching the script, now that OpenGFX (downloaded from in-game) is found:

Running ai/regression/tst_regression... passed!
Running ai/regression/tst_stationlist... passed!

@glx22
Copy link
Contributor

@glx22 glx22 commented Mar 7, 2019

Hmm I think config_dir should be an extra search dir appended before personal_dir. Would make more sense and keep the current behaviour too. Quote from readme:

Savegames will be relative to the config file only if there is no save/ directory in paths with higher priority than the config file path, but autosaves and screenshots will always be relative to the config file. Unless the configuration file is in $XDG_CONFIG_HOME/openttd, then all other files will be saved under $XDG_DATA_HOME/openttd.

@Berbe
Copy link
Contributor Author

@Berbe Berbe commented Mar 7, 2019

[EDIT] Mislead comment, keeping it only for reference


Current proposal breaks past behaviour of OpenTTD, as the location of the configuration file supplied with -c implies a configuration directory when resources & directories are created/searched for.
Worse, the configuration directory has precedence for every resource that are now not discovered/loaded anymore.

That is IMHO bad design, as documented directories shall be the only place the program shall ever write to, unless explicitely told otherwise. Widening the scope of the switch is not to be expected
With that idea in mind, -c should only provide the configuration file and not imply anything else, the working directory structure remaining the same.
This fix actually enforces that.

Now, it is not a satisfactory decision, as it breaks the backwards compitibility.
One idea is to transform the meaning of the -c switch from 'Loading a configuration file' to 'Changing working directory': This way, creation/search of files in a specified directory would be explicit, thus naturally expected.
In this working directory, the configuration file would naturally be seeked for and have higher precedence than other locations.
While the overall behaviour shall be the same, the semantics at hand imply an implementation which might differ quite a bit. I have no idea yet.

As a fallback, the new -c switch, normally accepting directories, could still be accepting the configuration file, implying the working dir around it, exactly as it is doing in the trunk version.

@Berbe
Copy link
Contributor Author

@Berbe Berbe commented Mar 8, 2019

TL; DR
I do not think my PR breaks how the game loads save files in anyway.
From what I witnessed, it merely changes/fixes the path for auto-downloaded content, which was searched in the same location as the supplied game configuration file.


Scratch my long previous comment.

It seems the intel I got about nice precendence of directories over others is simply not how the game is working at the moment.
As an example, savegames are loaded following that logic:

FILE *fh = (fop == SLO_SAVE) ? FioFOpenFile(filename, "wb", sb) : FioFOpenFile(filename, "rb", sb);

The paths are built following a combinations of values stored in those structures:
enum Subdirectory {

As I pointed out in my issue (and fixed here), the same Searchpath is not necessarily the same used for building paths to each Subdirectory. Combinations are context-dependent, and the logic hard to follow.

If you want to question potential side effects, it is best to take them one by one and either follow their logic into the code, statically, or just dynamically compare trunk & this version builds.

@stale
Copy link

@stale stale bot commented Apr 7, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Apr 7, 2019
@Berbe
Copy link
Contributor Author

@Berbe Berbe commented Apr 10, 2019

Ping?

@stale stale bot removed the stale label Apr 10, 2019
@stale
Copy link

@stale stale bot commented May 10, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label May 10, 2019
@Berbe
Copy link
Contributor Author

@Berbe Berbe commented May 14, 2019

Ping?

@stale stale bot removed the stale label May 14, 2019
@LordAro
Copy link
Member

@LordAro LordAro commented Apr 13, 2020

I'm going to close this one, as there's been no consensus on the correct solution, other than this is likely just a hack on top of a (much older) hack. Feel free to discuss in #7311

@LordAro LordAro closed this Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants