use XDG_CONFIG_HOME standard #137

bignaux · 2019-05-03T11:49:23Z

APP_PATH flag is to solve a problem on nixos where the path changed every update :
/nix/store/vzklpxmzhgm8wrnl4m6blrad2jw925il-caprice32-4.5.0
so it breaks user configuration for rom_path and resources_path. Btw, it makes debian patch unecessary too ;). They just have to add APP_PATH=/usr/share/caprice32 at make call.

ColinPitrat · 2019-05-09T09:33:55Z

This breaks the continuous build at the step that validates the build of debian package:
patching file cap32.cfg
Hunk #1 FAILED at 50.
Hunk #2 FAILED at 167.
2 out of 2 hunks FAILED

You should be able to fix it by modifying this file similarly to cap32.cfg:
caprice32/debian/patches/fix-install-path.patch

ColinPitrat

Globally I agree this is an improvement. Few remarks and questions.

ColinPitrat · 2019-05-09T09:35:08Z

doc/man6/cap32.6

+.br
+  - \fBcap32.cfg\fR file in the location pointed to by the XDG_CONFIG_HOME environment variable. If XDG_CONFIG_HOME is undefined, it will look at $HOME/.config/ as default XDG_CONFIG_HOME directory.
+.br
+  - \fB$HOME/cap32.cfg\fR for compatibility. 


Should be $HOME/.cap32.cfg

You should also mention $HOME/.config/cap32.cfg as it is also in the code.

redondant with "it will look at $HOME/.config/ as default XDG_CONFIG_HOME directory."

Oh right, but it's inaccurate. If XDG_CONFIG_HOME is defined, it will still look at $HOME/.config/cap32.cfg

src/cap32.cpp

cap32.cfg

makefile

src/cap32.cpp

bignaux · 2019-05-09T12:02:05Z

This breaks the continuous build at the step that validates the build of debian package:
patching file cap32.cfg
Hunk #1 FAILED at 50.
Hunk #2 FAILED at 167.
2 out of 2 hunks FAILED

You should be able to fix it by modifying this file similarly to cap32.cfg:
caprice32/debian/patches/fix-install-path.patch

since i could fix it and let travis test it, i can fix the debian package. Globally, we don't need to patch anymore.

ColinPitrat · 2019-05-09T16:08:07Z

src/cap32.cpp

+   // If not found, look for .cap32.cfg in the XDG_CONFIG_HOME of current user 
+   // If not found, look for .cap32.cfg in the default XDG_CONFIG_HOME of current user
+   std::string defaultConfig = ( getenv("XDG_CONFIG_HOME") != nullptr ) ? \
+      std::string(getenv("XDG_CONFIG_HOME")) : (std::string(getenv("HOME")) + "/.config") + "/cap32.cfg";


I'm not sure we can rely on HOME always being defined (typically on windows, but not only).
I realize previous code did, I suppose it was OK because it was always finding it locally.

I tried to think of multiple options, maybe the simplest would be something like that:

std::vector<std::pair<char*, std::string>> configPaths = { { "", args.cfgFilePath}, { chAppPath, "/cap32.cfg" }, { getenv("XDG_CONFIG_HOME"), "/cap32.cfg" }, ... } for(const auto& p: configFilename){ // Skip paths using getenv if it returned NULL (i.e environment variable not defined) if (!p.first) continue; std::string s = std::string(p.first) + p.second; if (access(s.c_str(), mode) == 0) std::cout << "Using configuration file" << (forWrite ? " to save" : "") << ": " << s << std::endl; return s; } }

And for the default, just hardcode "cap32.cfg" or "/etc/cap32.cfg", logging something to stdout or stderr as this should never be used ...

Sorry but i read a bit about such issue (multiplateform default config file) before i wrote this PR and i can tell that's not small code like our that solves every cases. People want better ? they fix it. I've no much time to spend on this. I solve my bugs for nixos and Magick2cpc.

i'm reading http://koshmaar.linuxpl.info/sdl_config/news.php
available here too https://github.com/Koshmaar/SDL_Config

Well, solving every cases is one thing. Here we're talking about making Caprice32 segfault systematically at startup on windows. Unless I miss something, previous code was working because it was never executing the getenv when running on windows because it takes the config file from the current directory before. But here it will execute the getenv no matter what resulting in a segfault.

Unfortunately, the windows CI on appveyor doesn't execute any e2e test. I should work on integrating it. But I suspect this would fail.

we could have #ifdef for different plateform default path settings. btw i'd like your feedback on sdl_config, if you're not against, i will try something, since it has substitution that was missing for my second part.

benefits : reduce code coverage, share code with other produts on configuration issue, add substitution etc. if you're not ready to increase your dependancies, you're not living in my world (no cost for a git module) yes ,last update is 2008 seems dead, but github was upload 8 months ago (people from SF) and i've already made an issue to see activity. As i did here .. answere here ? 6 days, so i'll look sdl-config that way. I first do a small contribution or legitimate question, then i wait. You still doesn't have merge my contribution, let's see , perharps sdl-config has not a lot of public interesting but i could make things there, i'm not worried. Free software died because of bad dictatorship not knowing on how deal with new contributor. I've plenty of non commenting PR on project, i know now their people are not able to manage a project (navit case). My PR merged i 'll do a new improvement, no news, i do nothing. My rule.

The configuration parsing/generation is 150 lines of code (.h and .cpp), didn't change much since its inception and never caused any issue so I'm not sure it's worth the overhead. A module that handles finding the configuration file too would be another story, but just parsing I'd advise against putting any effort into it.

As for the merging of the PR, I'm currently working on making the e2e tests run on appveyor so that I can verify whether this crashes systematically under windows, but I really think it does. I clearly can't merge a PR that would make the application crash systematically at startup for many users.

plus this not so great ones functions from cap32.cpp :
saveConfiguration() 67l
loadConfiguration() 114l
and variables substitutions in configuration would be great... btw, if you're not interessting by, no matter. Let write your test and i'll fix after failure. That way, we ensure a bit your test and my fix.

Those methods would remain if you were to use SDL_Config, just replacing (set|get).*Value with the SDL_Config equivalent. If not, then I'm missing something.

Appveyor build is updated to run the e2e_test too so you can sync your repository to see what it says.

Can you do a git pull and update your pull request to see what this gives on appveyor?

ColinPitrat · 2019-05-14T11:44:49Z

default.nix

@@ -0,0 +1,5 @@
+with import <nixpkgs> {};


Please add a component describing what this is.
Something like this I suppose:
# Package description for Nix (http://nixos.org)

ColinPitrat · 2019-05-28T08:50:21Z

Any news on this? I'm interested by the change so if you don't want to finish it, I can take it over.

bignaux · 2019-05-28T08:57:25Z

yes, very sorry to not reply. I'll have a look very soon.

ColinPitrat · 2019-09-01T09:26:09Z

Replaced by #139

ColinPitrat · 2020-03-14T11:35:18Z

Make a new tagged version release, for bignaux to finish the nixpkgs integration with this modification.

ColinPitrat · 2020-03-14T14:45:25Z

I created a v4.6.0 (release on-going).
Tell me if it works fine for you.
Thanks!

bignaux · 2020-03-14T15:37:31Z

It can now find the file but it won't create it for the moment, so user has still to cp /nix/store/4zwhyybwb73barllbll964dk2db6zmlw-caprice32-4.6.0/etc/cap32.cfg ~/.config/ to make cap32 look for it. Would be nicer to enable creation of the file, but that would be another task. For the moment i've no time for this, i just try to close the different topic i started month ago before my disease. Thanks, i did my duty for nixos NixOS/nixpkgs#82586

bignaux · 2020-03-14T15:44:10Z

Sorry, i try again, that doesn't write the file even with save.
Using configuration file: /home/genesis/.config/cap32.cfg
when save:
Using configuration file to save: /home/genesis/.config/cap32.cfg
but file is unmodified.

ColinPitrat · 2020-04-29T12:30:33Z

Sorry, didn't see your comment so far.
Is there any error message? What are the permissions on the file?
Can you try it with strace to see if the write succeeds or fail, and if it fails with which error?

ColinPitrat · 2020-04-29T12:32:45Z

I just tried on a file without write permissions and indeed, there's no error but the config is not saved.

…ound in #137

ColinPitrat · 2020-04-29T13:09:08Z

Now Caprice will display an error in the GUI and in the console when failing to save to a file (e.g non-writable).

bignaux added 2 commits May 2, 2019 18:05

add small default.nix for nix.

5bf621b

use XDG_CONFIG_HOME standard

572327f

bignaux force-pushed the master branch from 75f4312 to 572327f Compare May 3, 2019 15:30

add APP_PATH flag

d675f2d

bignaux force-pushed the master branch from b54aa0c to d675f2d Compare May 3, 2019 21:44

ColinPitrat reviewed May 9, 2019

View reviewed changes

src/cap32.cpp Outdated Show resolved Hide resolved

bignaux added 2 commits May 9, 2019 17:03

some fix according to review

653519e

fix: NULL -> nullptr

627176e

ColinPitrat reviewed May 9, 2019

View reviewed changes

This was referenced May 9, 2019

Please do realease tag. Koshmaar/SDL_Config#1

Open

infra-arcana: init at unstable-2019-04-22 NixOS/nixpkgs#61360

Closed

ColinPitrat reviewed May 14, 2019

View reviewed changes

ColinPitrat mentioned this pull request Sep 1, 2019

Modify and replace pull request #137 #139

Merged

ColinPitrat closed this Sep 1, 2019

Repository owner deleted a comment from bignaux Mar 14, 2020

ColinPitrat added a commit that referenced this pull request Apr 29, 2020

Display an error message when saving fails. This addresses an issue f…

54c7a3d

…ound in #137

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use XDG_CONFIG_HOME standard #137

use XDG_CONFIG_HOME standard #137

bignaux commented May 3, 2019 •

edited

Loading

ColinPitrat commented May 9, 2019

ColinPitrat left a comment

ColinPitrat May 9, 2019

ColinPitrat May 9, 2019

bignaux May 9, 2019

ColinPitrat May 9, 2019

bignaux commented May 9, 2019

ColinPitrat May 9, 2019 •

edited

Loading

bignaux May 9, 2019

bignaux May 9, 2019 •

edited

Loading

ColinPitrat May 9, 2019

bignaux May 9, 2019

bignaux May 9, 2019 •

edited

Loading

ColinPitrat May 10, 2019 •

edited

Loading

bignaux May 10, 2019

ColinPitrat May 10, 2019

ColinPitrat May 14, 2019

ColinPitrat May 14, 2019 •

edited

Loading

ColinPitrat commented May 28, 2019

bignaux commented May 28, 2019

ColinPitrat commented Sep 1, 2019

ColinPitrat commented Mar 14, 2020 •

edited

Loading

ColinPitrat commented Mar 14, 2020 •

edited

Loading

bignaux commented Mar 14, 2020

bignaux commented Mar 14, 2020

ColinPitrat commented Apr 29, 2020

ColinPitrat commented Apr 29, 2020

ColinPitrat commented Apr 29, 2020

use XDG_CONFIG_HOME standard #137

use XDG_CONFIG_HOME standard #137

Conversation

bignaux commented May 3, 2019 • edited Loading

ColinPitrat commented May 9, 2019

ColinPitrat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bignaux commented May 9, 2019

ColinPitrat May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bignaux May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bignaux May 9, 2019 • edited Loading

Choose a reason for hiding this comment

ColinPitrat May 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinPitrat May 14, 2019 • edited Loading

Choose a reason for hiding this comment

ColinPitrat commented May 28, 2019

bignaux commented May 28, 2019

ColinPitrat commented Sep 1, 2019

ColinPitrat commented Mar 14, 2020 • edited Loading

ColinPitrat commented Mar 14, 2020 • edited Loading

bignaux commented Mar 14, 2020

bignaux commented Mar 14, 2020

ColinPitrat commented Apr 29, 2020

ColinPitrat commented Apr 29, 2020

ColinPitrat commented Apr 29, 2020

bignaux commented May 3, 2019 •

edited

Loading

ColinPitrat May 9, 2019 •

edited

Loading

bignaux May 9, 2019 •

edited

Loading

bignaux May 9, 2019 •

edited

Loading

ColinPitrat May 10, 2019 •

edited

Loading

ColinPitrat May 14, 2019 •

edited

Loading

ColinPitrat commented Mar 14, 2020 •

edited

Loading

ColinPitrat commented Mar 14, 2020 •

edited

Loading