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

Use XDG specification for config files #94

Closed
sergiobenrocha2 opened this issue Mar 6, 2017 · 32 comments
Closed

Use XDG specification for config files #94

sergiobenrocha2 opened this issue Mar 6, 2017 · 32 comments
Assignees

Comments

@sergiobenrocha2
Copy link

@sergiobenrocha2 sergiobenrocha2 commented Mar 6, 2017

Right now there's a ~/.vbam directory, which should be located at ~/.config/vbam

Not directly related, but starter directory for search roms should be the home folder (~/), not ~/.vbam

@ZachBacon ZachBacon self-assigned this Mar 6, 2017
@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Feb 20, 2019

Any updates on this, @ZachBacon ? If it is still open (and just this issue not closed), would you review a PR? I am interested in working on this.

From what I see, there is only a single config file ~/.vbam/vbam.conf. We can do backward compatibility (check if old dir exists; if it does, use it. Otherwise, use new location), while setting the new default location to ${XDG_CONFIG_HOME:-$HOME/.config}/vbam/vbam.conf (I don't like the idea of changing the name of the configuration file, although I have seen on other software).

About this file, there is only one thing that we need to discuss: the [recent] section.

According to this, this type of info leans towards user data, although it is not actually that important to a user. We could split this to a second file, to be put, let's say, on ${XDG_DATA_HOME:-$HOME/.local/share}/vbam/vbam.ini.

Of course this is just a brainstorming; we can just keep a single file and use the XDG location.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Feb 21, 2019

@denisfa

I last worked on the config code a few years ago for better mac and windows support. On those platforms the file is in the correct location.

I did not change the location on unix because of backwards compatibility concerns you mentioned which I did not want to deal with at the time, I was only trying to get the program working on mac..

wxWidgets actually has support for XDG config file locations, this is being overridden currently.

If you are interested I'll give you some hints on where the relevant code is etc..

@rkitover rkitover added the config label Feb 21, 2019
@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Feb 21, 2019

@rkitover Thanks!

Sure, I could confirm some stuff! I believe I have to work all files using the macro DOT_DIR.
So the following files: ./src/common/ConfigManager.cpp, ./src/sdl/SDL.cpp and the header ./src/common/ConfigManager.h.

About the wxWidgets, I saw something like a FileLayout::FileLayout_XDG. Is this what you mean? I am not being able to test this now, because the file /usr/include/wx-3.0/wx/unix/stdpaths.h from my package distro does not contain this. It is missing some stuff, but I think I am probably looking at the wrong place.

The file for this should be ./src/wx/wxvbam.cpp.

What are your thoughts about the splitting? Or should we keep a single file for the sake of simplicity?

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Feb 24, 2019

@denisfa

In src/wx/wxvbam.cpp go to line 219

You see an #ifdef here, the first case is for windows and mac, the second is unix etc..

What we want to do here is use the same method on unix that we do on windows and mac.

This will also require changing get_config_path() which is called by GetConfigurationPath() line 36

This uses methods from wxStandardPaths. Note I'm linking the wx 3.1 version, not the 3.0 version.

Since 3.1, there is XDG support.

But if we want to support XDG paths with 3.0 (and maybe even 2.8) which is the stable version, we will need to do some more work. We will need to think about the best way to do this. Probably the easiest is to find the directory ourselves.

Also of note, on windows and mac we set the app name to visualboyadvance-m but on unix we set it to vbam, we should also make this consistent.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Feb 24, 2019

@denisfa

on the issue of splitting config and recent lists etc., let's just get the file in the right place first and then we can worry about this.

@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 1, 2019

Copied from #380:

@denisfa thank you, this looks great!

Some notes:

  • probably best to use __WXGTK__ instead of __LINUX__ to check for linux (and freebsd, etc., don't forget they still exist! smiling_imp )

I see. I focused on Linux too much, but at least this is a quick fix.

  • the ConfigManager stuff, we just need to check that the SDL port still works on windows, I will do this and make adjustments if necessary. To build the SDL port pass -DENABLE_SDL=ON to cmake.

The code affected was protected by #if !defined(_WIN32) && !defined(__APPLE__). This should not be a problem for Windows build/run. But I should have tested the SDL for Linux too. I already got a message Error creating file /home/denisfa/.vbam/loz.sav. This makes sense since I removed the .vbam dir creation. On this topic, should we move this type of file to a XDG dir too? This feels like data for me, and we could use ${XDG_DATA_HOME:-$HOME/local/share}/visualboyadvance-m. What do you think?

  • instead of checking for .ini on linux and .conf elsewhere, let's just check for both, check for .ini first and rename to .conf if it doesn't exist already, would be nice to have more consistency between platforms anyway

So use .conf for all platforms? .ini and a fallback option .cfg are used for Windows and MacOS. .conf is for everything else.

  • thank you for testing on 2.8, I put in a lot of work to make 2.8 still work a couple years ago, good to know it still does

Someone is probably going to complain if it indeed stops, so no worry about that hahaha.

  • probably a good idea to show a warning about the config file being in the old location and where to move it to, e.g. with wxLogWarning()

I agree. My original idea was to just put on release notes or something, but we can and should be nicer to users.

  • we cannot do any automated testing at this time, there are too many memory corruption bugs and I can't even run --help in the CI without segfaults

About this, when I build the original master branch and ran ./visualboyadvance-m --help or ./visualboyadvance-m --print-cfg-path, I got Segmentation fault (core dumped). Is this what you mean? Or is this a possible example?

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 1, 2019

The code affected was protected by #if !defined(_WIN32) && !defined(APPLE). This should not be a > problem for Windows build/run. But I should have tested the SDL for Linux too. I already got a message > Error creating file /home/denisfa/.vbam/loz.sav. This makes sense since I removed the .vbam dir creation. > On this topic, should we move this type of file to a XDG dir too? This feels like data for me, and we could > use ${XDG_DATA_HOME:-$HOME/local/share}/visualboyadvance-m. What do you think?

This may make it difficult for the user to find, it would be better to use the same strategy as the GUI and put the battery save file .sav into the same directory where the ROM file was loaded from.

The SDL port is not a very high priority, but we should ensure it is at least functional. Some people apparently use it for various things.

So use .conf for all platforms? .ini and a fallback option .cfg are used for Windows and MacOS. .conf is for > everything else.

Actually now that I think about it, we should probably use .ini on all platforms and rename the other variants. The reason being that the canonical MFC builds, which we also need to resurrect, used .ini. This way we have at least some config file portability.

About this, when I build the original master branch and ran ./visualboyadvance-m --help or .
/visualboyadvance-m --print-cfg-path, I got Segmentation fault (core dumped). Is this what you mean?
Or is this a possible example?

Exactly, that is an example. The app is riddled with memory errors, mostly in the core. Currently I have travis set up to just check that everything builds. We could maybe see what kind of tests mGBA does and do something similar later, I dunno.

@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 1, 2019

This may make it difficult for the user to find, it would be better to use the same strategy as the GUI and put the battery save file .sav into the same directory where the ROM file was loaded from.

I did some testing here and I found out the order of priority for .sav is this (GUI):

  1. The path (if) given by BatteryDir on vbam.conf, then fallback to $HOME/.vbam ;
  2. The ROM folder, then fallback to $HOME/.vbam;

The fallback occurs when the first dir is not writable by the user. If both dirs are not writable, then we get a error message box about not being able to create temp dirs and save the configuration file. So this is handled ok.

My suggestion towards this is to change the fallback into a XDG dir. We keep the order. The SDL Port uses 1. only. We could add the ROM folder before the fallback. I think this is your idea. So we get, for both wx and SDL:

  1. The path (if) given by BatteryDir on vbam.conf, the ROM folder and then fallback to $HOME/.vbam;

Actually now that I think about it, we should probably use .ini on all platforms and rename the other variants. The reason being that the canonical MFC builds, which we also need to resurrect, used .ini. This way we have at least some config file portability.

This is doable for new configs. This would mean the XDG Dir conf file should be .ini too.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 2, 2019

The logic for this is in GameArea::recompute_dirs() in panel.cpp and it is as you said.
The same logic is used for save states.
I'm agree with using the XDG data dir for the fallback, there is no need for a message either in this case.

~/.local/share is fine because battery saves are (supposed to be) platform independent.

I'd like to use the same logic for the SDL port, but that's a low priority.

I don't remember why I made the windows and mac config files .conf instead of .ini, but that was a mistake.
If possible we should both write new configs as .ini and detect and rename an existing .conf file.to .ini as well.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 2, 2019

Save states on the other hand may not be platform-independent, so would that be ~/.local/lib or ~/.local/lib64 then?

@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 6, 2019

I'd like to use the same logic for the SDL port, but that's a low priority.

The SDL port looks easy to set this up, once we define everything.

I don't remember why I made the windows and mac config files .conf instead of .ini, but that was a mistake.

All platforms should use vbam.ini. The way it is done for Windows and MacOS is to check first for vbam.ini and then vbam.cfg. I believe we can just remove the .cfg since it is used "only if vbam.ini does not exist and vbam.cfg exists". For this case, the user needed to manually rename from .ini to .cfg. If that is legacy of some type, we might need a migration path.

Save states on the other hand may not be platform-independent, so would that be ~/.local/lib or ~/.local/lib64 then?

What do you mean here? The Linux one does not work on Windows etc? Each platform has a "XDG equivalent". We can code this, if necessary (although wxWidgets does this already for Windows and MacOS; for Unix will be after 3.1).

The path (if) given by BatteryDir on vbam.conf, the ROM folder and then fallback to $HOME/.vbam;

This was a bit weird for me. The SDL port feels really different from the WX one; for instance, if the conf file does not exist, then it is also not created (so default values all around). Some code is all Linux/MacOS only but it is not protected by macros. This has stuff like the file sep "/" hardcoded. The macro EXE_NAME only has one use, and it is in a #ifdef _WIN32 protected, despite being defined to both Windows and others.

The variable homeDir is set to 0 to Windows and MacOS in ConfigManager.cpp. This makes all code of dirs creation not available for these platforms. But these are always created for Linux, nevertheless (even if not used, since the config might not be saved).

Overall, doing this will very likely lead to duplicated code (one for WX and another for SDL port). Is this intended? I have never worked with SDL in my life, so I may be missing a lot here.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 6, 2019

What do you mean here? The Linux one does not work on Windows etc? Each platform has a "XDG equivalent". We can code this, if necessary (although wxWidgets does this already for Windows and MacOS; for Unix will be after 3.1).

So, on a classical UNIX system you have:

/usr/share

and

/usr/lib

share is for platform independent files, like documentation and data, so that this directory can be e.g. mounted over nfs on a different architecture processor machine.

and lib is supposed to be for architecture-dependent files, like actual dynamic libraries etc..

So, we would assume that

~/.local/share

would be architecture-independent files, that a user could copy to a different architecture. While things like

~/.local/lib
~/.local/lib64

would be architecture-dependent files.

I don't know for sure if our state files are architecture-dependent or not, they may not be, we'll need to test this more I guess.

But this is what I was talking about.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 6, 2019

On mac and windows we can use the same directory and not worry about it I think.

@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 6, 2019

share is for platform independent files, like documentation and data, so that this directory can be e.g. mounted over nfs on a different architecture processor machine.

and lib is supposed to be for architecture-dependent files, like actual dynamic libraries etc.

I see. I believe we should have no problems with this.

On mac and windows we can use the same directory and not worry about it I think.

They use the same place for user data and user config. wxWidgets already set the right location for them.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 6, 2019

I need to check if the save states are actually binary incompatible between architectures (and by architectures I really mean intel/amd 32 and 64 bit.)

rkitover added a commit that referenced this issue Mar 7, 2019
Followup on #383.

Signed-off-by: Rafael Kitover <rkitover@gmail.com>
@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 7, 2019

One change I did to #383: 6a98f3c

I'll make a few other minor changes as part of another thing I'm working on now to fix rpi plugins.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 7, 2019

@denisfa if you are going to do any experiments with windows, I wrote this install guide for msys2:

https://gist.github.com/rkitover/d008324309044fc0cc742bdb16064454

rkitover added a commit that referenced this issue Mar 7, 2019
Store the full relative path to found `.rpi` plugins, relative to the
standard Plugins directory, as specified by wxWidgets.

This fixes the problem of plugins being in a subdirectory while only the
basename was stored, making the plugins unusable.

This is done by using `wxFileName::GetFullPath()` instead of
`wxFileName::GetFullName()` with a relative filename instance.

Make a `GetPluginsDir()` method on the app class to simplify getting
this directory, and for possible future overrides.

Also make some minor, functionally equivalent changes to
`get_config_path()` in `wxvbam.cpp`:

- use the new `GetPluginsDir()` method for the plugins directory when
  building the config file search path

- print the XdgConfigDir on all platforms, since the function works on
  all platforms

- make a `add_nonstandard_path` macro which duplicates the `add_path`
  macro for wxWidgets standard paths but for any arbitrary string path

- use `wxFileName` methods to make the XDG config directory path instead
  of string concatenation

Signed-off-by: Rafael Kitover <rkitover@gmail.com>
@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 7, 2019

@rkitover Thanks!

Let me confirm something: are windowX, windowY, fullScreen, fsWidth, fsHeight the geometry of the main app? They are not being used on the WX port, but I wonder if these are the vars that I wanted for a state file configuration.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 7, 2019

fsWidth and fsHeight I believe are holdovers from the dead code that allowed you to actually switch the resolution etc. for fullscreen, this isn't very useful on modern systems. The other ones should maybe be implemented, I don't know.

@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 8, 2019

@rkitover I manage to get a working code for saving/loading state data, although I don't think it is elegant at all. I have it here. Is this something that you could help me here? Or should I submit a PR and we go from there? (my branch denisfa:xdg-state-dir)

Also, about Windows, there is a cleanup needed, mostly because of how some code is no longer protected by #ifdef directives. I will be working on fixing this soon.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 8, 2019

@denisfa I left some comments on your commit.

Also when I was talking about saving states, I meant a different thing, although this is also good.

The emulator has a function to save its state, in the File -> Save state and File -> Load state menus. These are snapshot of the emulator state to restart a game at the same spot.

@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 9, 2019

@rkitover Thanks! I will work on it!

Yeah, my wording was ambiguous. But at least you could understand that I meant the GUI geometry. The [Geometry] and the [Recent] were the sections that I wanted to save to a different place (not sure the name of the file yet) due to them being more likely to be changed often than the rest of the config. They are classified by some as state data.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 9, 2019

I need to bring the mingw travis slaves back up so we have a test for compilability for windows. I had to bring them down because the mxe debian package repository went offline. If it's still offline I will just redo them using an mxe docker image or something.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 9, 2019

mingw build jobs on travis are fixed

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 9, 2019

I was not able to do the --help check on travis yet because for some reason the binary exits 255, it doesn't segfault anymore but still has this weird exit code, will try to fix this.

@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 9, 2019

I am sure this is related to the cleanup I need to do. I will also be working on this now.

@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 9, 2019

I am trying to test, but I am having some issues. How do you generate the exe that you can double-click on Windows? Using the MSYS I can build and run, but when I try clicking the exe I get all sorts of errors. This go from missing libs, to directX broken (???).

When I run from the MSYS shell, I get a popup window displaying the --help information. Not sure if this is common for windows or not, but it is really weird seeing something like this when I am linux-only.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 10, 2019

For me --help does not display at all from the msys shell.

As for how to run the executable built with msys2, you can either do:

./visualboyadvance-m.exe [path-to-rom]

from the terminal.

or add c:\msys64\mingw64\bin to your PATH environment variable (mingw32 for 32 bit builds.)

I prefer the first approach to not put potentially unwanted things in PATH.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 10, 2019

The other option with msys2 builds is to use something like e.g. dependencywalker to get all the .dlls and copy them to the .exe directory.

If you want a static build, you can use mxe in linux (or a docker image) or my build scripts on linux.

rkitover added a commit that referenced this issue Mar 11, 2019
* Add support to save/load geometry options for GUI window.

* Refactor code to use wxWidgets functions to get window geometry.

* Call update_opts() from ::OnSize and ::OnMove functions.
rkitover added a commit that referenced this issue Mar 14, 2019
* Add migration support for 'vbam.cfg' to 'vbam.ini' on MacOS and Windows.

* Cleanup from XDG Base Dir code.

* Set home to NULL after using free().
@denisfa

This comment has been minimized.

Copy link
Collaborator

@denisfa denisfa commented Mar 15, 2019

I need to check if the save states are actually binary incompatible between architectures (and by architectures I really mean intel/amd 32 and 64 bit.)

I did some testing about this with the help of virtual machines. I am running am AMD x86_64 and use QEMU to check others. The save states worked. My tests were like this: save a game state on one machine; send to other via scp; load the state.

The game was LoZ Oracle of Seasons, and it worked perfectly on all machines.

If this is correct, what else do we need to do to close this? I wanted to split the configuration file, but the current code already support the XDG Base Dir Spec.

@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 15, 2019

I'm fine with closing this unless you wanted to make any other enhancements, but these can be tracked elsewhere.

@rkitover rkitover added fixed and removed partially fixed labels Mar 15, 2019
@rkitover rkitover assigned rkitover and unassigned ZachBacon Mar 15, 2019
@rkitover rkitover closed this Mar 15, 2019
@rkitover

This comment has been minimized.

Copy link
Collaborator

@rkitover rkitover commented Mar 15, 2019

And by the way, thank you very much for doing all of this, this will definitely help keep us in the distro packages.

rkitover added a commit that referenced this issue Mar 18, 2019
* Apply save order for save states and batteries.
The order for save state/battery:
1. StateDir / BatteryDir;
2. The path of the current loaded game;
3. XDG Base Dir fallback.

* Use XDG Base Dir fallback to save screenshots and recordings.

* Apply search order for all dirs except recording (not implemented yet) of SDL port.
The order for battery/save state/screenshot is:
1. StateDir/BatteryDir/ScreenshotDir;
2. The path of the current loaded game;
3. XDG Base Dir (or equivalent) fallback.

* Refactor code.

* Fix freeing and setting pointer to NULL of SDL port.
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.