snapcraft.yaml: use build-base and adopt-info, rm builddeb plugin #7904

anonymouse64 · 2019-12-13T13:54:26Z

We can modernize the snapd snap by setting build-base to core, which allows the snap to be built with the modern snapcraft, using multipass or lxd (or natively with --destructive-mode). This also means we need to drop usage of build in favor of override-build, since in modern snapcraft the build keyword is removed entirely.

Finally, we can eliminate usage of the x-builddeb custom plugin and just use the same commands as part of the override-build snippet.

Also drive-by update to HACKING.md fixing typo from before

Note that this still does not yet set type: snapd because the store is still not ready for the transition, but we should land this anyways and make that change later.

cjp256 · 2019-12-13T14:03:16Z

snapcraft.yaml

@@ -29,9 +25,25 @@ passthrough:
 # https://forum.snapcraft.io/t/5547/10
 parts:
  snapd:
-    # FIXME: this should probably go upstream
-    plugin: x-builddeb


👍 I think doing this in override-build is much nicer!

stolowski · 2019-12-13T14:04:36Z

I haven't reviewd it yet, but I suspect the type: is still a blocker per #7092

cjp256

Looks good!

snapcraft.yaml

anonymouse64 · 2019-12-13T14:11:18Z

As per SU, I need to drop type: snapd from this because the store isn't ready for that yet, but the rest of the changes here should be good to go

anonymouse64 · 2019-12-13T19:12:42Z

I ended up force-pushing this since it's small enough to re-review I think. Also fixed the tests.

I think this can fully supercede #7092 and when the store is ready we can submit a follow-up to set type: snapd.

mvo5

Thank you! This looks great.

mvo5 · 2019-12-18T10:18:45Z

One minor issue (not a blocker) - if I run this twice without a "snapcraft clean" in between I get a strange error on my 19.10 host:

dpkg-shlibdeps: error: no dependency information found for /root/stage/lib/x86_64-linux-gnu/libc.so.6 (used by debian/snapd/usr/lib/snapd/snap-seccomp)
Hint: check if the library actually comes from a package.
dh_shlibdeps: dpkg-shlibdeps -Tdebian/snapd.substvars debian/snapd/usr/bin/snapfuse debian/snapd/usr/bin/snap debian/snapd/usr/lib/snapd/snap-confine debian/snapd/usr/lib/snapd/snap-seccomp debian/snapd/usr/lib/snapd/snap-bootstrap debian/snapd/usr/lib/snapd/snap-gdb-shim debian/snapd/usr/lib/snapd/snap-repair debian/snapd/usr/lib/snapd/snapd debian/snapd/usr/lib/snapd/snap-failure debian/snapd/usr/lib/snapd/snap-discard-ns debian/snapd/usr/lib/systemd/system-environment-generators/snapd-env-generator debian/snapd/lib/systemd/system-generators/snapd-generator returned exit code 2
debian/rules:102: recipe for target 'binary' failed
make: *** [binary] Error 2
dpkg-buildpackage: error: debian/rules binary gave error exit status 2
Failed to run 'override-build': Exit code was 2.

anonymouse64 · 2019-12-18T14:11:40Z

@mvo5 I'll look into that, we can probably fix that easily in this PR somehow

anonymouse64 · 2019-12-18T16:25:55Z

@mvo5 that issue has been fixed, should fully support iterative re-builds like that now

stolowski · 2019-12-19T12:49:16Z

build-aux/snap/snapcraft.yaml

+adopt-info: snapd
+# the base is only needed here for snapcraft to check the ELF sections of
+# binaries inside the snap, which is an unnecessary check for snapcraft itself
+base: core


I think this should be build-base: core, no?

With just build-base: core snapcraft errors like this https://pastebin.ubuntu.com/p/PhMkTmKcRw/

I believe this is a bug in snapcraft where it assumes that if there is a build-base defined then there must also be a base defined. Should the snapd snap not declare a base?

No, this was n'acked in my original PR, see #7092 (comment)

stolowski · 2019-12-19T13:59:46Z

Marking blocked until base:core is clarified.

anonymouse64 · 2019-12-19T14:58:53Z

@stolowski yes you're correct sorry I forgot about that situation. I have filed a bug against snapcraft for this: https://bugs.launchpad.net/snapcraft/+bug/1857019

anonymouse64 · 2019-12-19T17:27:57Z

@stolowski hmm, looking at the fix that snapcraft team implemented in #7092, that fix relies upon type: snapd to be defined in order to not need to define base: core. So here since we are still missing type and also have build-base: core, that doesn't work and as such the reason the other PR was green was because there was a type: snapd.

While unfortunate, I don't think we can build with the "modern" snapcraft with multipass/lxd here (and also with build-aux/snap) until the store is ready for type: snapd or until snapcraft implements yet another work-around 😕

stolowski · 2019-12-20T14:58:44Z

@stolowski hmm, looking at the fix that snapcraft team implemented in #7092, that fix relies upon type: snapd to be defined in order to not need to define base: core. So here since we are still missing type and also have build-base: core, that doesn't work and as such the reason the other PR was green was because there was a type: snapd.

Ah, right, I forgot that these things were interconnected... Oh well...

anonymouse64 · 2020-02-10T16:33:25Z

Note that I updated this to use build-base instead of base, so this PR will likely not successfully build a snap until https://bugs.launchpad.net/snapcraft/+bug/1857019 is fixed, however I'm told that will be fixed shortly.

anonymouse64 · 2020-02-13T15:22:16Z

At @sergiusens request, I tested this against snapcraft edge and it worked successfully. Specifically all I tested was running the snapd-snap spread test with this patch:

diff --git a/tests/main/snapd-snap/task.yaml b/tests/main/snapd-snap/task.yaml
index 4a7022bb3f..6310b6e85f 100644
--- a/tests/main/snapd-snap/task.yaml
+++ b/tests/main/snapd-snap/task.yaml
@@ -8,7 +8,8 @@ priority: 100
 
 prepare: |
     echo "Install snapcraft from candidate"
-    snap install snapcraft --candidate --classic
+    # TODO: move this back to candidate
+    snap install snapcraft --edge --classic
 
 restore: |
     cd "$PROJECT_PATH"

(and I had a bad rebase earlier that had just deleted the contents of the x_builddeb.py file, but not the full file itself somehow, so I fully deleted it and force pushed again).

stolowski

Looks good to me, thanks for pursuing it!

@stolowski

We can modernize the snapd snap by setting base to core, which allows the snap to be built with the modern snapcraft, using multipass or lxd (or natively with --destructive-mode). This also means we need to drop usage of build in favor of override-build, since in modern snapcraft the build keyword is removed entirely. Also, with a modern snapcraft, we can set the license without using passthrough, and we also should migrate to using snapcraftctl set-version, which is the preferred way to set the version in override-pull. We can also now eliminate usage of the x-builddeb custom plugin and just use the same commands as part of the override-build snippet. The test is now updated to build the snap natively with --destructive-mode, the lxd provider isn't quite stable enough for us to rely on it here and run the tests on other OS's. Unfortunately, we are still not able to set type: snapd yet, since the store isn't ready, so just add that there as a TODO. Thanks to @stolowski and @cmatsuoka for some of these changes! Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

This avoids the warnings about all the go files and such inside snap/ Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

… builds Leaving these set leads to the debian build system including files from $SNAPCRAFT_STAGE in the build and using those libraries, etc. which it should not be doing. Without this, after building successfully once, re-building the snap without cleaning would fail like this: dpkg-shlibdeps: error: no dependency information found for /root/stage/lib/x86_64-linux-gnu/libc.so.6 (used by debian/snapd/usr/lib/snapd/snap-seccomp) Hint: check if the library actually comes from a package. dh_shlibdeps: dpkg-shlibdeps -Tdebian/snapd.substvars debian/snapd/usr/bin/snapfuse debian/snapd/usr/bin/snap debian/snapd/usr/lib/snapd/snap-confine debian/snapd/usr/lib/snapd/snap-seccomp debian/snapd/usr/lib/snapd/snap-bootstrap debian/snapd/usr/lib/snapd/snap-gdb-shim debian/snapd/usr/lib/snapd/snap-repair debian/snapd/usr/lib/snapd/snapd debian/snapd/usr/lib/snapd/snap-failure debian/snapd/usr/lib/snapd/snap-discard-ns debian/snapd/usr/lib/systemd/system-environment-generators/snapd-env-generator debian/snapd/lib/systemd/system-generators/snapd-generator returned exit code 2 debian/rules:102: recipe for target 'binary' failed make: *** [binary] Error 2 dpkg-buildpackage: error: debian/rules binary gave error exit status 2 Failed to run 'override-build': Exit code was 2. because libc.so.6 was included in the linker search path during compiling when it should just be using the one from the build environment instead. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

This was somehow undone in one of the force pushes... Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

codecov-io · 2020-02-17T14:52:43Z

Codecov Report

Merging #7904 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7904      +/-   ##
==========================================
- Coverage   80.84%   80.84%   -0.01%     
==========================================
  Files         707      707              
  Lines       56445    56445              
==========================================
- Hits        45634    45633       -1     
  Misses       7277     7277              
- Partials     3534     3535       +1

Impacted Files	Coverage Δ
httputil/useragent.go	`82.85% <0%> (ø)`	⬆️
cmd/snap-seccomp/main.go	`67.12% <0%> (ø)`	⬆️
osutil/synctree.go	`76.82% <0%> (-2.44%)`	⬇️
interfaces/policy/policy.go	`95.94% <0%> (ø)`	⬆️
overlord/ifacestate/helpers.go	`80.22% <0%> (ø)`	⬆️
overlord/ifacestate/handlers.go	`66.62% <0%> (ø)`	⬆️
cmd/snap/cmd_debug_state.go	`67.51% <0%> (+0.63%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4a45e9...5c6b927. Read the comment docs.

…-modern

mvo5

Thank you!

mvo5 · 2020-02-20T15:07:24Z

Snapcraft in candidate suports this now so I remove the "blocked" label.

zyga · 2020-02-20T16:13:35Z

build-aux/snap/snapcraft.yaml

+      sudo apt-get build-dep -y ./
+      ./get-deps.sh --skip-unused-check
+      # set version after installing dependencies so we have all the tools here
+      snapcraftctl set-version $(./mkversion.sh --output-only)


Suggested change

snapcraftctl set-version $(./mkversion.sh --output-only)

snapcraftctl set-version "$(./mkversion.sh --output-only)"

Happy to see follow ups or if you are feeling lucky, commit this suggestion and wait for green.

zyga · 2020-02-20T16:14:20Z

build-aux/snap/snapcraft.yaml

+      # TODO: should we unset $PATH to not include $SNAPCRAFT_STAGE too?
+      unset LD_FLAGS
+      unset LD_LIBRARY_PATH
+      # if we are root, disable tests


Saying why we disable tests would be more useful. I see what the code does but am not the wiser.

sure I'll add this in a followup

zyga

LTGM but please see comments inline.

anonymouse64 requested review from stolowski and mvo5 December 13, 2019 13:54

cjp256 reviewed Dec 13, 2019

View reviewed changes

anonymouse64 added the ⛔ Blocked label Dec 13, 2019

cjp256 approved these changes Dec 13, 2019

View reviewed changes

snapcraft.yaml Outdated Show resolved Hide resolved

anonymouse64 force-pushed the feature/snapcraft-modern branch 2 times, most recently from 80f531b to ed26d4b Compare December 13, 2019 19:11

anonymouse64 mentioned this pull request Dec 13, 2019

packaging: use snapd type and snapcraft 3.x #7092

Closed

anonymouse64 removed the ⛔ Blocked label Dec 13, 2019

mvo5 approved these changes Dec 17, 2019

View reviewed changes

stolowski reviewed Dec 19, 2019

View reviewed changes

stolowski added the ⛔ Blocked label Dec 19, 2019

anonymouse64 force-pushed the feature/snapcraft-modern branch from 114b8a4 to 4180ba3 Compare February 10, 2020 14:50

anonymouse64 mentioned this pull request Feb 12, 2020

plugin handler: process elf files only if base is specified canonical/snapcraft#2926

Merged

4 tasks

anonymouse64 force-pushed the feature/snapcraft-modern branch from b757cdb to eb0bca0 Compare February 13, 2020 15:20

anonymouse64 changed the title ~~snapcraft.yaml: add type, build-base and adopt-info, rm builddeb plugin~~ snapcraft.yaml: use build-base and adopt-info, rm builddeb plugin Feb 13, 2020

anonymouse64 requested a review from stolowski February 13, 2020 15:23

stolowski approved these changes Feb 14, 2020

View reviewed changes

anonymouse64 added 5 commits February 17, 2020 08:20

snapcraft.yaml: mv to build-aux/snap

a321e9a

This avoids the warnings about all the go files and such inside snap/ Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

HACKING.md: add missing " in snap-confine example

83879d3

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

snapcraft.yaml: change base: core to build-base: core

5c6b927

This was somehow undone in one of the force pushes... Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 force-pushed the feature/snapcraft-modern branch from 2d7ffcd to 5c6b927 Compare February 17, 2020 14:20

anonymouse64 requested a review from mvo5 February 19, 2020 14:18

Merge remote-tracking branch 'upstream/master' into feature/snapcraft…

590e92c

…-modern

mvo5 approved these changes Feb 20, 2020

View reviewed changes

mvo5 removed the ⛔ Blocked label Feb 20, 2020

zyga reviewed Feb 20, 2020

View reviewed changes

zyga approved these changes Feb 20, 2020

View reviewed changes

anonymouse64 merged commit 4a967f1 into snapcore:master Feb 20, 2020

anonymouse64 deleted the feature/snapcraft-modern branch February 20, 2020 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snapcraft.yaml: use build-base and adopt-info, rm builddeb plugin #7904

snapcraft.yaml: use build-base and adopt-info, rm builddeb plugin #7904

anonymouse64 commented Dec 13, 2019 •

edited

cjp256 Dec 13, 2019 •

edited

stolowski commented Dec 13, 2019

cjp256 left a comment

anonymouse64 commented Dec 13, 2019

anonymouse64 commented Dec 13, 2019

mvo5 left a comment

mvo5 commented Dec 18, 2019

anonymouse64 commented Dec 18, 2019

anonymouse64 commented Dec 18, 2019

stolowski Dec 19, 2019

anonymouse64 Dec 19, 2019

stolowski Dec 19, 2019

stolowski commented Dec 19, 2019

anonymouse64 commented Dec 19, 2019

anonymouse64 commented Dec 19, 2019

stolowski commented Dec 20, 2019

anonymouse64 commented Feb 10, 2020

anonymouse64 commented Feb 13, 2020

stolowski left a comment

codecov-io commented Feb 17, 2020 •

edited

mvo5 left a comment

mvo5 commented Feb 20, 2020

zyga Feb 20, 2020

zyga Feb 20, 2020

anonymouse64 Feb 20, 2020

zyga left a comment

	snapcraftctl set-version $(./mkversion.sh --output-only)
	snapcraftctl set-version "$(./mkversion.sh --output-only)"

snapcraft.yaml: use build-base and adopt-info, rm builddeb plugin #7904

snapcraft.yaml: use build-base and adopt-info, rm builddeb plugin #7904

Conversation

anonymouse64 commented Dec 13, 2019 • edited

cjp256 Dec 13, 2019 • edited

Choose a reason for hiding this comment

stolowski commented Dec 13, 2019

cjp256 left a comment

Choose a reason for hiding this comment

anonymouse64 commented Dec 13, 2019

anonymouse64 commented Dec 13, 2019

mvo5 left a comment

Choose a reason for hiding this comment

mvo5 commented Dec 18, 2019

anonymouse64 commented Dec 18, 2019

anonymouse64 commented Dec 18, 2019

stolowski Dec 19, 2019

Choose a reason for hiding this comment

anonymouse64 Dec 19, 2019

Choose a reason for hiding this comment

stolowski Dec 19, 2019

Choose a reason for hiding this comment

stolowski commented Dec 19, 2019

anonymouse64 commented Dec 19, 2019

anonymouse64 commented Dec 19, 2019

stolowski commented Dec 20, 2019

anonymouse64 commented Feb 10, 2020

anonymouse64 commented Feb 13, 2020

stolowski left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 17, 2020 • edited

Codecov Report

mvo5 left a comment

Choose a reason for hiding this comment

mvo5 commented Feb 20, 2020

zyga Feb 20, 2020

Choose a reason for hiding this comment

zyga Feb 20, 2020

Choose a reason for hiding this comment

anonymouse64 Feb 20, 2020

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

anonymouse64 commented Dec 13, 2019 •

edited

cjp256 Dec 13, 2019 •

edited

codecov-io commented Feb 17, 2020 •

edited