Skip to content
Snippets Groups Projects

pressure-vessel: respect STEAM_COMPAT_FLAGS=search-cwd

Merged Ludovico de Nittis requested to merge wip/search_cwd into master
2 unresolved threads

If we have "search-cwd" in STEAM_COMPAT_FLAGS, and we are running the main program, we need to append to LD_LIBRARY_PATH the working directory of the game that we want to execute. Said directory is expected to be STEAM_COMPAT_INSTALL_PATH.

Fixes: #46 (closed)


This took me way more than expected. The whole process of how we decide what goes in LD_LIBRARY_PATH is a bit confusing and sparse in a lot of places. Then there was the session mode vs the single mode/relaunch to take into consideration.

In this MR there is my latest iteration where I used a similar check to the one in _v2-entry-point, to evaluate if we are running the main program, and condensed all the necessary bits in just a single place.

/cc @smcv

Edited by Simon McVittie

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • Resolved by Ludovico de Nittis

      I don't like this being in the adverb and done by default, because the purpose of the adverb is to run the command in a modified execution environment, like env and setpriv. I would prefer it to be like those commands in that it has no practical effect by default, but can be given options to modify the execution environment.

      For example, we use the adverb in various places in the SteamLinuxRuntime wrapper scripts, to run commands with a lock held.

      The whole process of how we decide what goes in LD_LIBRARY_PATH is a bit confusing and sparse in a lot of places.

      Yes. It's grown organically over time, we don't really have a very clear specification (although you got closer to one in T24167), and it will probably need to change it again for Flatpak support. It might make most sense to stop treating it as "just another environment variable" internally, and have "library search path" as a first-class-citizen variable - a GPtrArray of gchar *, or similar - that gets composed into a string LD_LIBRARY_PATH at the last possible moment.

      If we have to defer this change until we are ready to build LD_LIBRARY_PATH in a different or simpler way, or until we have decided whether relaunching is something we should always be doing, then so be it - it isn't a hugely high priority. I'll try to keep this in mind when I'm refactoring.

  • Ludovico de Nittis marked this merge request as draft

    marked this merge request as draft

  • Ludovico de Nittis added 223 commits

    added 223 commits

    Compare with previous version

  • added 1 commit

    • f8211395 - pressure-vessel: Add support for the STEAM_COMPAT_FLAGS=search-cwd

    Compare with previous version

  • mentioned in merge request steamlinuxruntime!33 (closed)

  • added 1 commit

    • 0ec97f20 - pressure-vessel: Add support for the STEAM_COMPAT_FLAGS=search-cwd

    Compare with previous version

  • Ludovico de Nittis marked this merge request as ready

    marked this merge request as ready

  • Ludovico de Nittis requested review from @smcv

    requested review from @smcv

  • Simon McVittie changed title from adverb: respect STEAM_COMPAT_FLAGS=search-cwd to pressure-vessel: respect STEAM_COMPAT_FLAGS=search-cwd

    changed title from adverb: respect STEAM_COMPAT_FLAGS=search-cwd to pressure-vessel: respect STEAM_COMPAT_FLAGS=search-cwd

    • This looks a lot better than the previous approach of doing it in the adverb.

      I'd like to at least seriously consider putting all the handling of this feature in pressure-vessel-wrap, so that pv-wrap parses STEAM_COMPAT_FLAGS itself and behaves accordingly.

    • The part that touches PvRuntime seems fine.

      I agree that --search-cwd doesn't seem very explicit about what we're doing.

      One idea: just parse STEAM_COMPAT_FLAGS by splitting it on commas. Don't have a PRESSURE_VESSEL_ environment variable, and don't have a command-line option either. This could be done either in PvRuntime or in pv-wrap.

      Another idea: have STEAM_COMPAT_FLAGS, together with a --enable-compat-flag=search-cwd, --disable-compat-flag=search-cwd override that takes precedence over STEAM_COMPAT_FLAGS.

      Either way, I think unknown compat flags should be ignored (with at most a g_info()).

    • Right, the less bash we have, the better :)

      I added the STEAM_COMPAT_FLAGS parsing in pv-wrap (probably safer to handle future flags?) and I avoided adding the enable and disable options simply because right now there isn't a use for them. But if in the future this changes, we can always add them at a later time.

    • Please register or sign in to reply
  • added 1 commit

    • 9ed64292 - pressure-vessel: Add support for the env var STEAM_COMPAT_FLAGS

    Compare with previous version

  • added 1 commit

    • 35e4e76f - pressure-vessel: Add support for the env var STEAM_COMPAT_FLAGS

    Compare with previous version

  • 730 * The check used is similar to the one in _v2-entry-point.
    731 * Try to keep them in sync.
    732 */
    733 static gboolean
    734 check_main_program (int argc,
    735 char *argv[])
    736 {
    737 gsize i;
    738 const gchar *session_id = g_getenv ("STEAM_COMPAT_SESSION_ID");
    739 if (session_id == NULL)
    740 /* Non-session mode, behaving like the main program */
    741 return TRUE;
    742
    743 for (i = 0; i < argc; i++)
    744 {
    745 if (g_strcmp0 (argv[i], "waitforexitandrun") == 0)
    • This is parsing command-line options that ought to be opaque to pressure-vessel, which I'm never a big fan of.

      It's also not actually going to work, I don't think, because Proton games (which don't need this behaviour!) do run pv-wrap as (simplified):

      pressure-vessel-wrap --option --option -- /path/to/proton waitforexitandrun /path/to/hl2.exe

      but the case where we actually need search-cwd is native Linux games, which run pv-wrap more like this:

      pressure-vessel-wrap --option --option -- /path/to/hl2.sh

      In the short term, I think we can detect the main program by g_getenv ("SteamAppId") being non-NULL.

      In the long term, I want to cut out most of the shell scripts and move more knowledge about the Steam compat-tool protocol into pressure-vessel itself, but I don't really have a design for that yet. Timo has talked about maybe introducing a v3 compat-tool protocol with explicit setup/open, run and teardown/close steps, which is something I have wanted for a long time, and perhaps moving the compat-tool protocol into pressure-vessel itself should wait for that.

    • I see. Doesn't that mean that the check for is_main in _v2-entry-point is wrong too?

      If I'm reading the script correctly, when we are in session mode, and we don't have waitforexitandrun in the main game, we are never going to set is_main to yes.

    • Doesn't that mean that the check for is_main in _v2-entry-point is wrong too?

      No, because we enter _v2-entry-point with a command-line like this:

      _v2-entry-point --verb=waitforexitandrun -- /path/to/proton waitforexitandrun /path/to/hl2.exe
                     |<--  parsed by v2ep     -->|<--treated as opaque from our point of view    -->|

      and it's the --verb option that _v2-entry-point is paying attention to.

      If _v2-entry-point passed a new option --steam-compat-v2-verb=waitforexitandrun to pv-wrap, then it would be OK for pv-wrap to respect that option - but I don't think it's worth plumbing that in right now.

    • Aahh you are right. I missed the --verb option.

      I changed the main program check to look at SteamAppId instead.

    • Please register or sign in to reply
  • added 1 commit

    • f32f230a - pressure-vessel: Add support for the env var STEAM_COMPAT_FLAGS

    Compare with previous version

  • Simon McVittie approved this merge request

    approved this merge request

  • Simon McVittie enabled an automatic merge when the pipeline for f32f230a succeeds

    enabled an automatic merge when the pipeline for f32f230a succeeds

  • Simon McVittie mentioned in commit dd7c036c

    mentioned in commit dd7c036c

  • Please register or sign in to reply
    Loading