Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: show that snap from snapd deb can be bundled #14551

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zyga
Copy link
Contributor

@zyga zyga commented Sep 30, 2024

Developers may want to bundle the snap executable from the snapd project into their application to get access to the snap APIs in a convenient form.

This test shows that this can be safely done for as long as the snapd deb is used as the source (not snapd snap) and that the ABI of snapd.deb and the application snap's base match.

Relates to changes made here: #14509

Jira: https://warthogs.atlassian.net/browse/SNAPDENG-32203

@zyga zyga force-pushed the feature/test-snap-LD_LIBRARY_PATH-SNAPDENG-32203 branch from c266f0f to 2bf9095 Compare September 30, 2024 10:53
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.87%. Comparing base (ac897ee) to head (96ec47f).
Report is 37 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14551      +/-   ##
==========================================
+ Coverage   78.85%   78.87%   +0.01%     
==========================================
  Files        1079     1082       +3     
  Lines      145615   145941     +326     
==========================================
+ Hits       114828   115109     +281     
- Misses      23601    23635      +34     
- Partials     7186     7197      +11     
Flag Coverage Δ
unittests 78.87% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Developers may want to bundle the snap executable from the snapd project
into their application to get access to the snap APIs in a convenient
form.

This test shows that this can be safely done for as long as the snapd
deb is used as the source (not snapd snap) and that the ABI of snapd.deb
and the application snap's base match.

Jira: https://warthogs.atlassian.net/browse/SNAPDENG-32203

Signed-off-by: Zygmunt Krynicki <[email protected]>
@zyga zyga force-pushed the feature/test-snap-LD_LIBRARY_PATH-SNAPDENG-32203 branch from 2bf9095 to 96ec47f Compare September 30, 2024 13:01
@zyga zyga added this to the 2.66 milestone Sep 30, 2024
@zyga zyga marked this pull request as ready for review September 30, 2024 13:59


prepare: |
cp "$(command -v snap)" test-snapd-sh-core24/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cp "$(command -v snap)" test-snapd-sh-core24/bin
cp -v "$(command -v snap)" test-snapd-sh-core24/bin

# configuration.
#
# shellcheck disable=SC2016
test-snapd-sh-core24.sh -c 'LD_PRELOAD=/lib/x86_64-linux-gnu/libz.so "$SNAP"/bin/snap' | MATCH 'Usage: snap <command> \[<options>...\]'
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure LD_PRELOAD is making any difference here if it's loading a library from base which matches the running system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the whole point. It's supposed to show you can do it in a very specific way.
We're not testing for properties of the snap binary from the snapd snap.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's nothing specific about it. We almost guarantee that taking a binary from 24.04 it keeps working when executed in a snap that uses core24, preload or not.



prepare: |
cp "$(command -v snap)" test-snapd-sh-core24/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

this could also copy the snap command the snapd snap and show that it fails to run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but then the test name would need to be different. I also don't know what that specific test would show. On some systems (without apparmor) it would actually work because it would load the dynamic linker from /snap/snapd/current

Copy link
Contributor

Choose a reason for hiding this comment

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

This test is running only on ubuntu-24.04, which means that if you copy a snap binary from the snap it will fail (or at least should), which is the exact scenario folks should not be doing. i.e. copying stuff from the snapd snap into their snaps.

@ernestl ernestl modified the milestones: 2.66, 2.67 Oct 1, 2024
Copy link
Collaborator

@ernestl ernestl left a comment

Choose a reason for hiding this comment

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

Nitpicks, and needs discussion around Maciek's point.

# on ubuntu core, because /usr/bin/snap is the "snapd snap" version which is
# linked differently.
- ubuntu-24.04-64

Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra space

@@ -0,0 +1,30 @@
summary: snaps bundling /bin/snap from snapd.deb can do so
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: Summary should start with capital, and preferable end with '.'

@ernestl
Copy link
Collaborator

ernestl commented Oct 1, 2024

Changing milestone to 2.67. It is not important to have merged when cutting the release.

@bboozzoo
Copy link
Contributor

bboozzoo commented Oct 9, 2024

discussed the test with @zyga, he'll push an update.

@ernestl
Copy link
Collaborator

ernestl commented Oct 21, 2024

As per feedback from @zyga, removing from milestone 2.67 in favour of landing layout improvements.

@ernestl ernestl removed this from the 2.67 milestone Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants