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

Wrong determination of personal directory #7311

Open
Berbe opened this issue Mar 2, 2019 · 7 comments

Comments

@Berbe
Copy link
Contributor

@Berbe Berbe commented Mar 2, 2019

Version of OpenTTD

Tested on trunk 90a4737
Some calls to the DEBUG macro have been added to track the problem down & produce the output pastes that follow.

Expected result

Normal game runs fine after OpenGRF baseset has been downloaded from ingame, and thus resides in ~/.openttd/content_download/baseset/
The regression test execution should find it aswell.

A patched regression run (one where the custom configuration file -c ai/regression/regression.cfg has been removed) shows as follow:

dbg: [misc] Scanning for tars
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/baseset/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/baseset/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/baseset/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /usr/local/share/games/openttd/baseset/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/content_download/baseset/, recursive = true
dbg: [misc] Found dir in tar: opengfx-0.5.2/
dbg: [misc] Found file in tar: opengfx-0.5.2/license.txt (17987 bytes, 1024 offset)
dbg: [misc] Found file in tar: opengfx-0.5.2/opengfx.obg (9998 bytes, 19968 offset)
dbg: [misc] Found file in tar: opengfx-0.5.2/ogfxt_toyland.grf (622079 bytes, 30720 offset)
dbg: [misc] Found file in tar: opengfx-0.5.2/ogfxe_extra.grf (911491 bytes, 653312 offset)
dbg: [misc] Found file in tar: opengfx-0.5.2/ogfxc_arctic.grf (287331 bytes, 1565696 offset)
dbg: [misc] Found file in tar: opengfx-0.5.2/ogfx1_base.grf (2698330 bytes, 1853952 offset)
dbg: [misc] Found file in tar: opengfx-0.5.2/ogfxi_logos.grf (253846 bytes, 4553216 offset)
dbg: [misc] Found file in tar: opengfx-0.5.2/ogfxh_tropical.grf (472020 bytes, 4807680 offset)
dbg: [misc] Found file in tar: opengfx-0.5.2/readme.txt (14584 bytes, 5280256 offset)
dbg: [misc] Found file in tar: opengfx-0.5.2/changelog.txt (24910 bytes, 5295616 offset)
dbg: [misc] Found tar '/home/<user>/.openttd/content_download/baseset/OpenGFX-0.5.2.tar' with 10 new files
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /usr/local/share/games/openttd/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/content_download/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /usr/local/share/games/openttd/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/content_download/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /usr/local/share/games/openttd/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/content_download/data/, recursive = true
dbg: [misc] Scan complete, found 1 files

No error is reported.

Actual result

Regression test execution fails to find the OpenGRF archive, which yields an error.
Moreover, debug output shows the following:

dbg: [misc] Scanning for tars
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/baseset/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/baseset/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/baseset/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /usr/local/share/games/openttd/baseset/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = ai/regression/content_download/baseset/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /usr/local/share/games/openttd/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = ai/regression/content_download/gm/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /usr/local/share/games/openttd/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = ai/regression/content_download/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/.openttd/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /home/<user>/source/bin/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = /usr/local/share/games/openttd/data/, recursive = true
dbg: [misc] /home/<user>/source/src/fileio.cpp:1363 ScanPath extension = .tar, path = ai/regression/content_download/data/, recursive = true
dbg: [misc] Scan complete, found 0 files

Steps to reproduce

Execute make regression on Linux
To showcase the run not failing to find the baseset, edit bin/ai/regression/run.sh to remove the -c ai/regression/regression.cfg argument to the call to openttd.

Quick analysis

The following lines of fileio.cpp determine the config_dir variable content:

1184     if (_config_file != NULL) {
1185         config_dir = stredup(_config_file);

Then, the following line replicate config_dir in _personal_dir, which looks problematic:

1237     {
1238         _personal_dir = config_dir;
1239     }
@Berbe

This comment has been minimized.

Copy link
Contributor Author

@Berbe Berbe commented Mar 7, 2019

I discovered the following (the line numbers are the ones of some added DEBUG macro calls, but a good indication of location in the file):

src/fileio.cpp:1092 DetermineBasePaths _searchpaths[SP_PERSONAL_DIR]: /home/<user>/.openttd/
src/fileio.cpp:1239 DeterminePaths _personal_dir: ai/regression/, _searchpaths[SP_PERSONAL_DIR] = /home/<user>/.openttd/
src/fileio.cpp:1254 DeterminePaths _searchpaths[SP_AUTODOWNLOAD_DIR]: ai/regression/content_download/

Why is _personal_dir initialized with config_dir instead of the expected value correctly populated in _searchpaths[SP_PERSONAL_DIR] at:

_personal_dir = config_dir;
?

@PeterN

This comment has been minimized.

Copy link
Member

@PeterN PeterN commented Mar 7, 2019

Hmm, yes, If the config path is overridden (as it is for regression testing) then we ignore the XDG directories. I'm not sure why...

@Berbe

This comment has been minimized.

Copy link
Contributor Author

@Berbe Berbe commented Mar 7, 2019

I am not even talking about the XDG directories, here: _searchpaths[SP_PERSONAL_DIR_XDG] is another entry of the array, and it does not get activated for me when my problem is triggered.

Moreover, if #if defined(WITH_XDG_BASEDIR) && defined(WITH_PERSONAL_DIR) triggers, the fallback pointed out before at L1224 does not occur.

Berbe added a commit to Berbe/OpenTTD that referenced this issue Mar 7, 2019
Berbe added a commit to Berbe/OpenTTD that referenced this issue Mar 7, 2019
@stale

This comment has been minimized.

Copy link

@stale stale bot commented May 7, 2019

This issue has been automatically marked as stale because it has not had any activity in the last two months.
If you believe the issue is still relevant, please test on the latest nightly and report back.
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 7, 2019
@stale stale bot closed this May 14, 2019
@Berbe

This comment has been minimized.

Copy link
Contributor Author

@Berbe Berbe commented May 14, 2019

?

@Berbe

This comment has been minimized.

Copy link
Contributor Author

@Berbe Berbe commented May 18, 2019

Could this issue be kept open until associated PR is dealt with?

@LordAro LordAro reopened this Dec 2, 2019
@LordAro LordAro removed the stale label Dec 2, 2019
@frosch123

This comment has been minimized.

Copy link
Member

@frosch123 frosch123 commented Dec 2, 2019

One of the key behaviours of the "-c" option is: When OpenTTD writes files without user interaction for selecting a location, it only writes files relative to the config file (i.e. relative to the -c directory). This includes autosaves, screenshots and download content.

That is, it is intended behaviour that SP_AUTODOWNLOAD_DIR refers to the directory of the "-c" option:
_searchpaths[SP_AUTODOWNLOAD_DIR] = str_fmt("%s%s", _personal_dir, "content_download" PATHSEP);

So, this issue boils down to

  • Content files are scanned in the search-paths, as specified in docs/directory_structure.md. The config file and "-c" option do not add to these search-paths.
  • Automatically written files (autosave, screenshots, ...) are put relative to the config file or "-c" option.
  • Content download belongs into both categories, but OpenTTD can only handle one path for this.
  • In theory there could be 3 places for content_download:
    • SP_PERSONAL_DIR_XDG/content_dowload (~/.local/share/openttd/content_download)
    • SP_PERSONAL_DIR/content_download (~/.openttd/content_download) (when built and run without XDG support somewhen in the past)
    • _personal_dir/content_download (-c option + "/content_download")

Two possible solutions:

  • 3 separate SP_AUTODOWNLOAD_DIR_xyz.
  • Remove SP_AUTODOWNLOAD_DIR and essentially replace the scan tuple (Searchpath, Subdirectory) with a triple (Searchpath, AutoDownload, Subdirectory). Or maybe make "recursive=False" scan a "content_download" nevertheless.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.