snapd
snapd copied to clipboard
snapcore
cmd/snap: create the XDG_RUNTIME_DIR folder
Currently, the XDG_RUNTIME_DIR folder isn't created when a snap is run, which requires in several cases to include a helper script to create it if it doesn't exist.
This behavior violates the fd.o base directory spec https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html as commented here https://forum.snapcraft.io/t/rethinking-how-we-handle-xdg-runtime-dir/22223
This MR fixes this.
Thanks for helping us make a better snapd! Have you signed the license agreement and read the contribution guide?
Digging through the version control history, there is disabled code in snap-confine that used to do this:
https://github.com/snapcore/snapd/blob/32f06e8f70b3f2861940f7596f2de832a71006ee/cmd/snap-confine/user-support.c#L50-L71
It was disabled in https://github.com/snapcore/snapd/pull/2430, with more discussion in https://github.com/snapcore/snap-confine/pull/195. All things considered, I think trying to create the directory from snap-confine was ill considered, so if we are fixing this problem I'd suggest that the PR also delete the disabled snap-confine code.
As for where the code should go, I think there are two options that would make sense:
- where you've done it in
snap runusing the environment it is about to invoke snap-confine with. - in snap-exec: the loader that snap-confine runs within the sandbox to start the application.
I'd tend to favour (2) from the point of view that it is running with lower privilege, and sees the file system as the snap will. But both options seem somewhat reasonable.
It would also be good to add a spread test to demonstrate that the directory is being created for the snap and has the expected permissions.
I see the tests failing and it could be caused by the change, please check the test results.
Failing when the folder couldn't be created was making fail some tests, because the "global" XDG_RUNTIME_DIR folder didn't exist, so it couldn't create the inner one. Now it's fixed.
@sergio-costas, it is looking good, but snapd tests are executed in arm as well, could you please make the snap available for all the architectures?
error: snap "test-snapd-xdg-dir" is not available on edge for this architecture (arm64) but exists on other architectures (amd64).
We will need for all the architectures when we run in in the lab boards for beta validation.
@sergio-costas please make it available for all the architectures, when we run this test on beta validation we need it in all the architectures.
@sergiocazzolato I tried to create an "all" snap, but inside it only allows "amd64". I've been able to build AMD64 and ARM64 specifying manually the architecture. How can I create a snap for "all" the architectures?
@sergio-costas did you try to add this in the snapcraft.yaml?
architectures:
- build-on: amd64 run-on: all
@sergiocazzolato Yes, and it returns this error:
Bad snapcraft.yaml content:
- string type expected (in field 'architectures[0]')
- extra field 'run-on' not permitted in 'architectures[0]' configuration
Full execution log: 'XXXXXXXXXXX.local/state/snapcraft/log/snapcraft-20230322-182511.010197.log'
@sergio-costas I see test snaps in snapd/tests/lib/snaps/store/ dir with those configurations. Could you please try both?
architectures:
- all
and
architectures:
- build-on: amd64
run-on: all
Ok, I think that I found how to do it:
architectures:
- build-on: [ amd64, arm64 ]
build-for: [ all ]
@sergio-costas, I see this error now, you need to publish it for all the architectures in the store
- snap install --edge test-snapd-xdg-dir error: snap "test-snapd-xdg-dir" is not available on edge for this architecture (i386) but exists on other architectures (amd64, arm64).
@sergiocazzolato Maybe I have to add "i386" in the list of architectures...
@sergiocazzolato Ok, it seems that i386 is only supported in "core" and "core18"... I'll try to change it.
Codecov Report
Merging #12583 (b9ebb2a) into master (8897c00) will increase coverage by
0.23%. Report is 383 commits behind head on master. The diff coverage is100.00%.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
@@ Coverage Diff @@
## master #12583 +/- ##
==========================================
+ Coverage 78.61% 78.84% +0.23%
==========================================
Files 991 1013 +22
Lines 122813 125910 +3097
==========================================
+ Hits 96553 99279 +2726
- Misses 20186 20429 +243
- Partials 6074 6202 +128
| Flag | Coverage Δ | |
|---|---|---|
| unittests | 78.84% <100.00%> (+0.23%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Files Changed | Coverage Δ | |
|---|---|---|
| cmd/snap/cmd_run.go | 61.18% <100.00%> (+0.23%) |
:arrow_up: |
... and 158 files with indirect coverage changes
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
@sergio-costas if you have addressed the requested changes, could you please ask to re-review this one?
@pedronis This should be ready...
the agreement was for James to look at it first again
@pedronis I mean "It is ready for another review". Sorry for the misunderstanding O:-)
@jhenstridge When you have some spare time, can you review this again, please?
@jhenstridge Can you review this when you have some spare time, please?