I haven't tested whether this works with persistent overrides, so that's worth making sure that it works. I think it should just work fine though.

Confirmed working:

Some issues were raised in previous attempt that may need to be addressed here as well, see #2555 (comment) and below.

How would the compatibility work with older version of a flatpak? If a package maintainer put this in finish-args and the user use a flatpak version that doesn't understand it. Given how long some distro version that never update flatpak (because they are hostile to it) are being used, this is likely to be unusable.

How would the compatibility work with older version of a flatpak?

That's another valid question.

Conversely, if a pre-existing app has a literal $ in its --env (which might seem unlikely, but I wouldn't be entirely surprised if there was a dual-architecture x86_64/i386 app like Steam with LD_PRELOAD=/usr/${LIB}/something.so or LD_PRELOAD=/app/${LIB}/something.so), what's the backward compatibility story there? Adding --env-expanded=... would likely be a more backward-compatible way to introduce the new feature.

flatpak run --env=PATH=$PATH:$HOME/.local/bin org.my.app

The fact that this specific case doesn't work is good. Because it's not supposed to work. binaries in $HOME are not supposed to work inside the sandbox.

If a package maintainer put this in finish-args and the user use a flatpak version that doesn't understand it

I do have an answer for that, which is the same as every other new sandboxing parameter: if this feature was added, then the app maintainer should either not use it until the necessary Flatpak version is ubiquitous (a timescale of years), or set their app's minimum version to a Flatpak that has this feature and accept that it will not be installable on oldLTS distros.

or set their app's minimum version to a Flatpak that has this feature and accept that it will not be installable on oldLTS distros.

Which mean not on flathub. This totally defeat the whole purpose.

Some issues were raised in previous attempt that may need to be addressed here as well, see #2555 (comment) and below.

What @Erick555 said. Before anyone spends time on reviewing the implementation, I think we'll need a summary of the design that explains how it addresses the concerns from previous attempts at this, and I would not want to merge this without approval from @alexlarsson.

Ah, I didn't realize there was a previous implementation, and I probably should have checked to see if it had been attempted before 😅

From Alex in the other PR:

So, the proper way imho is to apply all the elements in the envp, in order, expanding only against things already in the envp (i.e. no recursive expansion). Then you have to pick some arbitrary order (alphabetical?) in order to make this reproducible. On top of that we need to handle $ in the value as an escaped $, and possibly add the expand-if-variable-set syntax.

And from @smcv:

How would the compatibility work with older version of a flatpak?

That's another valid question.

Conversely, if a pre-existing app has a literal $ in its --env (which might seem unlikely, but I wouldn't be entirely surprised if there was a dual-architecture x86_64/i386 app like Steam with LD_PRELOAD=/usr/${LIB}/something.so or LD_PRELOAD=/app/${LIB}/something.so), what's the backward compatibility story there? Adding --env-expanded=... would likely be a more backward-compatible way to introduce the new feature.

I agree that if we want to move forward with this, maybe we do want to separate this into a new flag --env-expanded, and apply expanded variables after everything else sets envp (--env flags being the one I know of)

I would also have to have a set order, alphabetical seems fine to me, as long as we document it. Handling escaped $ might be as simple as evaluating \$ to $.

Regular expansion is not expressive enough to allow PATH extension when the original variable was empty or unset. You need something like the bash ${variable:+value} syntax.

We start to get more in the weeds with stuff like this, but I see now why this feature doesn't exist yet in flatpak

or set their app's minimum version to a Flatpak that has this feature and accept that it will not be installable on oldLTS distros.

Which mean not on flathub. This totally defeat the whole purpose.

Actually, I envisioned this to be more useful for local overrides that a user might set, I hadn't even considered it being set in a flatpak manifest, but it makes sense that developers might want to do that.

Should be ready for an initial review now. Decided to go with a new flag --env-expanded, which is evaluated after --env (therefore overriding it), and expansion of every --env-expanded value happens before applying them, to avoid any ordering issues. I also updated test-exports and test-override to cover some of --env-expanded.

I'll address what I ended up going with regarding #2555 (comment) :

We need some form of escapes to handle the case where you actually want a $ in the env value

I have not made any escapes for --env-expanded, but should be easy to implement if that's wanted. Since it's a new flag, it shouldn't negatively affect any existing usage of --env

Regular expansion is not expressive enough to allow PATH extension when the original variable was empty or unset. You need something like the bash ${variable:+value} syntax.
Now we're parsing like a mini language...

I don't have any special ${variable:+value} syntax, instead simply assuming that any usage of this expects either the value to be previously set or for that not to matter. For an empty PATH, this would mean that

--env-expanded=PATH='$PATH:/my/new/path`

would evaluate to

PATH=:/my/new/path

which I don't actually know what that will do 🤷

expansion is order sensitive, whereas the env handling doesn't really have a well defined order (i.e. its a hash table, not an array of settings in FlatpakContext). This means that something like A=foo:$B + B=bar can result in A being foo: or foo:bar, depending on the order of expansion.

This one was pretty easy. I just made expansion happen before applying all the expansions in envp. We also avoid the idea of recursive expansion, simplifying it even more.

Let me know your thoughts!

which I don't actually know what that will do 🤷

This is how it is specified in POSIX:

A zero-length prefix is a legacy feature that indicates the current working directory. It appears as two adjacent characters ( "::" ), as an initial preceding the rest of the list, or as a trailing following the rest of the list. A strictly conforming application shall use an actual pathname (such as .) to represent the current working directory in PATH.

I tested Bash and it seems conformant:

$ cat greet
#!/usr/bin/env bash
echo hello

$ greet
bash: greet: command not found

$ PATH=":$PATH"

$ greet
hello

I'm not sure how it works for other <colon>-separated variables, but I won't be surprised if it copies this behavior.

If anyone is interested in this patch, some testing would be nice. From what I recall, this worked on my VM test setup, but edge cases could be different. 🤷