Code review comment for lp:~michihenning/unity-scopes-api/set-env-vars

Revision history for this message
Michi Henning (michihenning) wrote :

> TMPDIR is currently set to:
> setenv("TMPDIR", cache_dir_.c_str(), 1);
>
> Based on what you set XDG_DATA_HOME and that my recommendation was to set it
> to 'XDG_DATA_HOME=$HOME/.local/share', it seems that cache_dir_.c_str() is
> '~/.local/share/' which is not a writable path.

Not quite. cache_dir_ is $HOME/.local/share/unity-scopes/<scope_type>/<scope_id>

For example: /home/michi/.local/share/unity-scopes/leaf-net/com.triodia.scopes.michiscope

In other words, I was setting TMPDIR to the same location as cache_dir_, which is the scope's writable data directory.

> TMPDIR should be set like so:
>
> TMPDIR=$XDG_RUNTIME_DIR/confined-scopes/leaf-net/<'name' from click manifest>

I can't get at the click manifest. All I have is the scope ID. I *think* that's always the same as the click manifest name?

> We want to have TMPDIR be set to something in /run because if a scope doesn't
> clean up after itself, at least a reboot will remove the files (this is what
> we do for apps).

Yes, I agree with that, something under /run/user/<uid> is better, so it gets cleaned up eventually.

So, I could do:

/run/user/<uid>/confined-scopes/<scope_type>/<scope_id>

For example, TMPDIR would be:

/run/user/1000/confined-scopes/leaf-net/com.triodia.scopes.michiscope

Whether to substitute "leaf-net", "leaf-fs", or "aggregator" (or "unconfined", if we prefer that) would depend on what cache directory I found earlier: if the cache directory contains "leaf-fs", TMPDIR would contain "leaf-fs" too.

One other thing though… Does it makes sense to have XDG_RUNTIME_DIR set to /run/user/<uid>?
Considering that this is not a location the scope can write to, wouldn't it be better to set
XDG_RUNTIME_DIR to /run/user/1000/confined-scopes/leaf-net/com.triodia.scopes.michiscope,
and TMPDIR to /run/user/1000/confined-scopes/leaf-net/com.triodia.scopes.michiscope/tmp?

Aargh… What should an *unconfined* scope have in that path? Surely not "confined-scopes"?
Would it be better to use just "scopes" whether a scope is confined or not? E.g, for a leaf-net scope:

/run/user/1000/scopes/leaf-net/com.triodia.scopes.michiscope

And for an aggregator:

/run/user/1000/scopes/aggregator/com.triodia.scopes.michiscope (or possibly "unconfined" instead of "aggregator")

Would that work?

There is also the issue with deeply-nested paths under /run/user/<uid>. As you say, things under /run/user/<uid> may be periodically removed. I assume this means that I should create the directory hierarchy (whichever one we settle on) under /run/user/<uid> if it does not exist? (For now, I'll be creating the directories, with permission 0700.)

> Also, PATH and LD_LIBRARY_PATH are being unconditionally set. They should only
> be set in the manner you are setting them if they are not already set.

I wasn't sure whether it's a good idea to rely on the environment that is inherited by the scoperunner, which is why I smashed these two variables. I'll change the code to prepend to LD_LIBRARY_PATH and PATH instead.

« Back to merge proposal