Skip to content

Inherit flatpak-run environment from parent instance when spawning#5510

Merged
smcv merged 7 commits intoflatpak:mainfrom
doraskayo:flatpak-spawn-env
Oct 27, 2023
Merged

Inherit flatpak-run environment from parent instance when spawning#5510
smcv merged 7 commits intoflatpak:mainfrom
doraskayo:flatpak-spawn-env

Conversation

@doraskayo
Copy link
Copy Markdown
Contributor

@doraskayo doraskayo commented Sep 3, 2023

This PR propagates the environment in which the original flatpak run command was executed to the flatpak run command that spawns subsandboxes.

This implements the idea discussed in #5278.

Before

FLATPAK_GL_DRIVERS which is used to select a GL runtime extension to mount in the sandbox is not considered when setting up subsandboxes, resulting in the default GL extension being mounted in the subsandbox unexpectedly:

$ FLATPAK_GL_DRIVERS=mesa-git flatpak run --command=glxinfo org.freedesktop.Platform.GlxInfo | grep -o 'Mesa 23.*' | head -1
Mesa 23.3.0-devel (git-1ae3c40732)
$ FLATPAK_GL_DRIVERS=mesa-git flatpak run --command=flatpak-spawn org.freedesktop.Platform.GlxInfo glxinfo | grep -o 'Mesa 23.*' | head -1
Mesa 23.1.6 (git-0697ac0d75)

After

FLATPAK_GL_DRIVERS which is used to select a GL runtime extension to mount in the sandbox is now properly considered when setting up subsandboxes, resulting in the mesa-git GL extension being mounted in the subsandbox as expected:

$ FLATPAK_GL_DRIVERS=mesa-git flatpak run --command=glxinfo org.freedesktop.Platform.GlxInfo | grep -o 'Mesa 23.*' | head -1
Mesa 23.3.0-devel (git-1ae3c40732)
$ FLATPAK_GL_DRIVERS=mesa-git flatpak run --command=flatpak-spawn org.freedesktop.Platform.GlxInfo glxinfo | grep -o 'Mesa 23.*' | head -1
Mesa 23.3.0-devel (git-1ae3c40732)

CC: @smcv


  • instance: Extract lock creation logic and reverse checks

    This allows extending the instance ID allocation logic with less
    impact on readability.

    No change in behavior.

  • instance: Create private instance directory

    Unlike the instance directory, whose directory structure is
    considered public API and is mounted in the sandbox, the private
    instance directory is meant to hold private data or metadata about
    an instance for use by internal components.

    The private instance directory is not meant to be shared with any
    external component, and provides no guarantees about its structure
    or contents.

    While the public instance directory is named "<instance-id>", the
    private instance directory is named "<instance-id>-private". Both the
    public and private instance directories share the same parent
    directory.

    The private instance directory relies on the same lock file as the
    public instance directory, and both are garbage-collected together.

  • run: Save flatpak-run environment

    Save the environment in which flatpak-run was executed in the private
    instance directory.

    The environment is saved in "env -0" format.

  • utils: Add flatpak_parse_env_block()

    This function can be used to parse environment variable blocks in
    "env -0" format from a buffer. It performs a few format checks during
    its parsing and returns an error if an issue is found.

    When successful, it returns a string array containing each individual
    environment variable parsed from the buffer, in the same format as
    the return value of g_get_environ().

    The implementation of this function is based on
    flatpak_context_parse_env_block().

  • context: Use flatpak_parse_env_block()

    No change in behavior, except minor wording of the error message.

  • instance: Add flatpak_instance_get_run_environ()

    This private function returns the environment in which flatpak-run
    was executed for a given FlatpakInstance.

  • portal: Inherit flatpak-run environment from parent when spawning

    Instead of inheriting the portal's environment when spawning a
    subsandbox using flatpak-run, inherit the environment in which
    flatpak-run was originally executed for the parent instance.

    This means that environment variables that affect the sandbox setup
    of the parent instance now also propagate to the setup of
    subsandboxes, including "FLATPAK_GL_DRIVERS".

    Closes: [Bug]: Subsandbox should base its environment on the environment of the original flatpak run #5278

@smcv smcv self-requested a review September 4, 2023 11:16
@doraskayo doraskayo force-pushed the flatpak-spawn-env branch 2 times, most recently from 1e33167 to 3132d7c Compare September 5, 2023 11:11
Copy link
Copy Markdown
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some comments on the implementation, but this looks like a really good direction to be going in. Thank you for working on this!

@smcv smcv added bug reviewed needs work portal Issues involving builtin portals labels Sep 11, 2023
@smcv
Copy link
Copy Markdown
Collaborator

smcv commented Sep 11, 2023

I want to test this with Steam/pressure-vessel before landing it, but it seems "the same shape" as what I would have implemented myself if I had time, so I expect it to work fine with Steam/pressure-vessel.

@doraskayo
Copy link
Copy Markdown
Contributor Author

I handled some of the smaller comments. I'll handle the rest in the coming days.

@doraskayo
Copy link
Copy Markdown
Contributor Author

I pushed another set of changes to address a few of the review comments. I hope to find time to handle the rest tomorrow.

@doraskayo doraskayo force-pushed the flatpak-spawn-env branch 2 times, most recently from 3f8de3e to c824771 Compare September 17, 2023 23:23
@doraskayo
Copy link
Copy Markdown
Contributor Author

doraskayo commented Sep 17, 2023

I pushed another set of changes to handle a few more review comments. The remaining threads are all waiting for a reply.

With this last set of changes, I think the PR is ready for a second round of review.

@doraskayo doraskayo force-pushed the flatpak-spawn-env branch 2 times, most recently from b874b0f to cf2a00c Compare September 23, 2023 15:15
@doraskayo
Copy link
Copy Markdown
Contributor Author

@smcv, thanks for the thorough review! All review comments have been handled.

@doraskayo doraskayo requested a review from smcv September 23, 2023 15:31
@doraskayo doraskayo closed this Oct 25, 2023
@doraskayo doraskayo reopened this Oct 25, 2023
This allows extending the instance ID allocation logic with less
impact on readability.

No change in behavior.
Unlike the instance directory, whose directory structure is
considered public API and is mounted in the sandbox, the private
instance directory is meant to hold private data or metadata about
an instance for use by internal components.

The private instance directory is not meant to be shared with any
external component, and provides no guarantees about its structure
or contents.

While the public instance directory is named "<instance-id>", the
private instance directory is named "<instance-id>-private". Both the
public and private instance directories share the same parent
directory.

The private instance directory relies on the same lock file as the
public instance directory, and both are garbage-collected together.
Save the environment in which flatpak-run was executed in the private
instance directory.

The environment is saved in "env -0" format.
This function can be used to parse environment variable blocks in
"env -0" format from a buffer. It performs a few format checks during
its parsing and returns an error if an issue is found.

When successful, it returns a string array containing each individual
environment variable parsed from the buffer, in the same format as
the return value of g_get_environ().

The implementation of this function is based on
flatpak_context_parse_env_block().
No change in behavior, except minor wording of the error message.
This private function returns the environment in which flatpak-run
was executed for a given FlatpakInstance.
Instead of inheriting the portal's environment when spawning a
subsandbox using flatpak-run, inherit the environment in which
flatpak-run was originally executed for the parent instance.

This means that environment variables that affect the sandbox setup
of the parent instance now also propagate to the setup of
subsandboxes, including "FLATPAK_GL_DRIVERS".

Closes: flatpak#5278
@doraskayo
Copy link
Copy Markdown
Contributor Author

@smcv, is anything else needed from me here?

Unfortunately I won't be able to make code changes for a while, so hopefully the code is acceptable in its current form.

@smcv smcv removed the reviewed label Oct 27, 2023
Copy link
Copy Markdown
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to do some "live" testing on this with the Steam Linux Runtime, but it seems right. Thanks!

@smcv smcv removed the needs work label Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug portal Issues involving builtin portals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Subsandbox should base its environment on the environment of the original flatpak run

2 participants