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

Add XDG_STATE_HOME and HOST_XDG_STATE_HOME env variables #4478

Merged
merged 7 commits into from Nov 15, 2021

Conversation

lionirdeadman
Copy link
Contributor

@lionirdeadman lionirdeadman commented Oct 11, 2021

This gives new support for the new XDG_STATE_HOME addition to XDG_BASE_DIRS which allows applications to use this without breaking because they would assume $HOME/.local/state which may be unavailable to the flatpak

This adds it as .local/state as to make --persist=.local/state the same behaviour as in new flatpak. This in turn means that the transition should be seamless between old and new flatpak.

This also has the benefit of working if the application doesn't follow XDG spec thanks to --persist=.local/state.

This fixes #4477

common/flatpak-run.c Outdated Show resolved Hide resolved
common/flatpak-run.c Outdated Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented Oct 12, 2021

Add XDG_STATE_HOME and HOME_XDG_STATE_HOME env variables

Presumably you mean HOST_XDG_STATE_HOME?

smcv
smcv previously requested changes Oct 12, 2021
tests/testlib.c Outdated Show resolved Hide resolved
tests/testlibrary.c Outdated Show resolved Hide resolved
tests/testlib.c Outdated Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented Oct 12, 2021

NOTE : This has not been tested

Please do? That would have caught several mistakes.

@smcv
Copy link
Collaborator

smcv commented Oct 12, 2021

The Steam Flatpak app has had a lot of trouble with games/apps that blindly assume $XDG_CACHE_HOME, $XDG_CONFIG_HOME and $XDG_DATA_HOME will take their default values, for example writing a file to ~/.config/my.conf and expecting to be able to read it back from $XDG_CONFIG_HOME/my.conf. As a result, it overrides Flatpak's default handling of these directories so that instead of ~/.var/app/com.example.App/cache, .../config and .../data, it will use their defaults ~/.cache, ~/.config and ~/.local/share, to maximize the number of games/apps that will work as intended.

It's far too late for us to avoid that problem for XDG_CACHE_HOME, XDG_CONFIG_HOME, and XDG_DATA_HOME in Flatpak apps more generally, because by now there are probably apps that blindly assume ~/config and so on, and creating symlinks or bind-mounts between the two is a regression risk.

However, because XDG_STATE_HOME is a new feature, I would be inclined to bypass that whole mess by creating the state directory in the app-private home directory as ~/.local/state (instead of something like ~/state), so that regardless of whether an app properly respects $XDG_STATE_HOME or just hard-codes ~/.local/state, it will work either way.

That would also have the benefit that it's the same directory that would be used by apps built with flatpak build-finish --persist=.local/state --unset-env=XDG_STATE_HOME, which gives us a migration path for older versions of Flatpak.

@gasinvein
Copy link
Member

bypass that whole mess by creating the state directory in the app-private home directory as ~/.local/state (instead of something like ~/state), so that regardless of whether an app properly respects $XDG_STATE_HOME or just hard-codes ~/.local/state, it will work either way.

Note that there is no persistent app-private home directrory, unless the flatpak app has --persist=.; $HOME in sandbox is not normally mapped to $HOME/.var/app/$FLATPAK_ID on host.

@smcv
Copy link
Collaborator

smcv commented Oct 12, 2021

Note that there is no persistent app-private home directrory, unless the flatpak app has --persist=.; $HOME in sandbox is not normally mapped to $HOME/.var/app/$FLATPAK_ID on host.

I didn't mean to imply that it is. There is always an app-private directory: it's where config, cache and data are kept. It is not always mounted in the sandbox, but it always exists.

If apps want to use XDG_STATE_HOME on older Flatpak, then it is not going to work by default, regardless of what we might do in newer Flatpak. However, apps can make it work by having --persist=.local/share and --unset-env=XDG_STATE_HOME. If we are going to make XDG_STATE_HOME work by default in newer Flatpak, I would like it to be compatible with that.

@gasinvein
Copy link
Member

gasinvein commented Oct 12, 2021

I didn't mean to imply that it is. There is always an app-private directory: it's where config, cache and data are kept. It is not always mounted in the sandbox, but it always exists.

Ah, sorry, I was confused by ~/config in you previous message - assumed that ~ refers to $HOME and not to $HOME/.var/app/$FLATPAK_ID.

If apps want to use XDG_STATE_HOME on older Flatpak, then it is not going to work by default

This change seems trivial - why not backport it?

@smcv
Copy link
Collaborator

smcv commented Oct 12, 2021

This change seems trivial - why not backport it?

Not all distributions use our stable releases. Some distributions are using branches that we no longer support, and backporting individual security fixes themselves; other distributions are using branches that we do support, but backporting security fixes themselves instead of using the stable releases that we provide for this purpose. We cannot make new features available in distributions that go out of their way to avoid them, but app authors presumably want their apps to work on those distributions anyway.

Even for those distros that do take our stable releases as-is, we need to "pick our battles": the more new features we backport, the more likely those distros become to stop accepting our stable releases as being sufficiently low-risk.

@gasinvein
Copy link
Member

gasinvein commented Oct 12, 2021

I think that what could really help Steam flatpak a lot is an opt-in alternate XDG_*_HOME paths layout. E.g.

Path variable "Default layout" value "Alternate layout" value
$XDG_CONFIG_HOME $HOME/.var/app/$FLATPAK_ID/config $HOME/.var/app/$FLATPAK_ID/.config
$XDG_DATA_HOME $HOME/.var/app/$FLATPAK_ID/data $HOME/.var/app/$FLATPAK_ID/.local/share
$XDG_CACHE_HOME $HOME/.var/app/$FLATPAK_ID/cache $HOME/.var/app/$FLATPAK_ID/.cache
$XDG_STATE_HOME $HOME/.var/app/$FLATPAK_ID/state $HOME/.var/app/$FLATPAK_ID/.local/state

@smcv
Copy link
Collaborator

smcv commented Oct 12, 2021

I think that what could really help Steam flatpak a lot is an opt-in alternate XDG_*_HOME paths layout

For cache/config/data: yes that seems good, but it's out of scope for this MR (and it's a new feature, so the Steam Flatpak app will not be able to rely on it for several years).

For state, I think the right thing to do is to always use the layout with .local/state, so that it's one less point of divergence.

@gasinvein
Copy link
Member

gasinvein commented Oct 12, 2021

For cache/config/data: yes that seems good, but it's out of scope for this MR (and it's a new feature, so the Steam Flatpak app will not be able to rely on it for several years).

For state, I think the right thing to do is to always use the layout with .local/state, so that it's one less point of divergence.

If such selectable layouts are to be implemented, I think it could be made on per-variable basis (e.g. --xdg-layout=cache=alternate) with different defaults: default to current layout for config/cache/data, but to "alternate" layout for state. This way we'll keep consistency between all the xdg variables and reduce divergence.

@smcv
Copy link
Collaborator

smcv commented Oct 12, 2021

If such selectable layouts are to be implemented, I think it could be made on per-variable basis

That sounds bad for test coverage: 4 boolean settings means 16 combinations, and realistically nobody is going to test them all. I'd rather have a single configuration that is heavily tested and works, or perhaps eventually one boolean to use .config, .local/share, .cache (but, again, that's out of scope for this MR).

@lionirdeadman lionirdeadman changed the title Add XDG_STATE_HOME and HOME_XDG_STATE_HOME env variables Add XDG_STATE_HOME and HOST_XDG_STATE_HOME env variables Oct 16, 2021
@lionirdeadman
Copy link
Contributor Author

I've now tested that it works, made it use .local/state as per discussion here and fixed all of the previous reviews. Sorry for not testing it from the start, I was just hoping this would be a simple stupid copy-paste job (and that was a mistake).

tests/testlibrary.c Outdated Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented Oct 21, 2021

This should really have automated test coverage, but I noticed we don't have test coverage for XDG_DATA_HOME, etc. either. I've added test coverage for this whole family of variables as an extra commit on your branch.

@smcv smcv requested a review from alexlarsson October 21, 2021 17:46
@smcv
Copy link
Collaborator

smcv commented Oct 21, 2021

I've expanded this to create ~/.var/app/<appID>/.local/state, and expanded the tests to assert that we create all these directories for each app.

tests/testlib.c Outdated Show resolved Hide resolved
@smcv smcv self-requested a review October 21, 2021 18:16
@mwleeds mwleeds dismissed smcv’s stale review October 25, 2021 18:11

Requested changes have been made

@mwleeds
Copy link
Collaborator

mwleeds commented Oct 25, 2021

The Steam Flatpak app has had a lot of trouble with games/apps that blindly assume $XDG_CACHE_HOME, $XDG_CONFIG_HOME and $XDG_DATA_HOME will take their default values, for example writing a file to ~/.config/my.conf and expecting to be able to read it back from $XDG_CONFIG_HOME/my.conf. As a result, it overrides Flatpak's default handling of these directories so that instead of ~/.var/app/com.example.App/cache, .../config and .../data, it will use their defaults ~/.cache, ~/.config and ~/.local/share, to maximize the number of games/apps that will work as intended.

It's far too late for us to avoid that problem for XDG_CACHE_HOME, XDG_CONFIG_HOME, and XDG_DATA_HOME in Flatpak apps more generally, because by now there are probably apps that blindly assume ~/config and so on, and creating symlinks or bind-mounts between the two is a regression risk.

However, because XDG_STATE_HOME is a new feature, I would be inclined to bypass that whole mess by creating the state directory in the app-private home directory as ~/.local/state (instead of something like ~/state), so that regardless of whether an app properly respects $XDG_STATE_HOME or just hard-codes ~/.local/state, it will work either way.

Would it work either way? Isn't ~/.local/state/ not the same as $HOME/.var/app/$FLATPAK_ID/.local/state ?

That would also have the benefit that it's the same directory that would be used by apps built with flatpak build-finish --persist=.local/state --unset-env=XDG_STATE_HOME, which gives us a migration path for older versions of Flatpak.

It's kind of unfortunate that using $HOME/.var/app/$FLATPAK_ID/.local/state for XDG_STATE_HOME diverges from the pattern of the other XDG config/data/cache dirs, but I guess it's worth it for the backwards compatibility. Do we know if any apps are actually using --persist=.local/share and --unset-env=XDG_STATE_HOME with flatpak build-finish or is the idea that they may want to do that at some point in the future when $XDG_STATE_HOME use is more widespread but they still want to be compatible with old versions of Flatpak?

tests/testlibrary.c Outdated Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented Oct 25, 2021

Would it work either way? Isn't ~/.local/state/ not the same as $HOME/.var/app/$FLATPAK_ID/.local/state ?

Correct, unless the app uses --persist=.local/state, in which case they are the same.

That would also have the benefit that it's the same directory that would be used by apps built with flatpak build-finish --persist=.local/state --unset-env=XDG_STATE_HOME, which gives us a migration path for older versions of Flatpak.

It's kind of unfortunate that using $HOME/.var/app/$FLATPAK_ID/.local/state for XDG_STATE_HOME diverges from the pattern of the other XDG config/data/cache dirs, but I guess it's worth it for the backwards compatibility. Do we know if any apps are actually using --persist=.local/share and --unset-env=XDG_STATE_HOME with flatpak build-finish or is the idea that they may want to do that at some point in the future when $XDG_STATE_HOME use is more widespread but they still want to be compatible with old versions of Flatpak?

I am not aware of apps that are using --persist=.local/state, but I'd like to be able to recommend --persist=.local/state --unset-env=XDG_STATE_HOME as something they can do to avoid needing a dependency on 1.13.x (or 1.12.x if we stretch the definition of a bug fix).

[edit: state, not share. It's unfortunate that this expansion to the basedir spec used a word with very similar letters...]

@mwleeds
Copy link
Collaborator

mwleeds commented Oct 25, 2021

Okay, this LGTM aside from the couple things I pointed out. With those fixed, we can merge unless you think we should wait for Alex to review it. And then we will want to update docs.flatpak.org since that mentions the default values of these XDG dirs: flatpak/flatpak-docs#311

lionirdeadman and others added 7 commits October 26, 2021 00:02
This gives new support for the new XDG_STATE_HOME addition to XDG_BASE_DIRS
which allows applications to use this without breaking because they would
assume $HOME/.local/state which may be unavailable to the flatpak

This adds it as .local/state as to make --persist=.local/state the same behaviour
as in new flatpak. This in turn means that the transition should be seamless between
old and new flatpak.

This also has the benefit of working if the application doesn't follow XDG spec thanks
to --persist=.local/state.

This fixes flatpak#4477

[smcv: Don't call nonexistent g_get_user_state_dir(); fix a reference
to XDG_STATE_DIR]
Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
This is placed at the same location that would be used by
the --persist=.local/state option.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
@alexlarsson
Copy link
Member

lgtm

@alexlarsson alexlarsson merged commit e2b36d3 into flatpak:master Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XDG_STATE_HOME is not set
5 participants