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

build-aux/snap/snapcraft.yaml: use build-packages, don't fail dirty builds #11197

Conversation

anonymouse64
Copy link
Member

Followup to #11188.

We don't want to fail dirty builds since this means that no local builds of the
snapd snap can be performed unless all changes are committed, which is
unfortunate and adds unnecessary steps for snapd developers during development.

Instead when we detect that a build is dirty, include the information inside a
file that is put into the snap that can be inspected later on. This will make
it easy to spot what changed during the build of a dirty snap if for some
reason we ever have those sneak by into the released channels of the published
snap.

Also use build-packages for build dependencies instead of manual apt install
commands.

Also make the GitHub Actions step to build the snapd snap fail when it produces a snap
with a dirty git version to prevent committing changes via PRs which result in dirty git
versions. This may not 100% stop the issue because some changes may only show up in
i.e. LP builds somehow, but should catch most issues going forward.

These changes produce for this exact change without being committed the
following file inside the snap:

dirty git tree during build detected:
On branch feature/build-packages-instead-of-override-pull
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   build-aux/snap/snapcraft.yaml

Untracked files:
  (use "git add <file>..." to include in what will be committed)

        2.53.3-changelog.txt
        2.53.4-changelog.txt

no changes added to commit (use "git add" and/or "git commit -a")
diff --git a/build-aux/snap/snapcraft.yaml b/build-aux/snap/snapcraft.yaml
index cd800ce..3aec80f 100644

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
--- a/build-aux/snap/snapcraft.yaml
+++ b/build-aux/snap/snapcraft.yaml
@@ -30,16 +30,14 @@ parts:
     plugin: nil
     source: .
     build-snaps: [go/1.13/stable]
+    # these packages are needed to call mkversion.sh in override-pull, all other
+    # dependencies are installed using apt-get build-dep
+    build-packages:
+      - git
+      - dpkg-dev
     override-pull: |
       snapcraftctl pull
       # set version, this needs dpkg-parsechangelog (from dpkg-dev) and git
-      sudo -E apt-get install -y dpkg-dev git
-      if sh -x ./mkversion.sh --output-only | grep "dirty"; then
-          echo "dirty git tree during build detected:"
-          git status
-          git diff
-          exit 1
-      fi
       snapcraftctl set-version "$(./mkversion.sh --output-only)"
       # Ensure that ./debian/ packaging which we are about to use
       # matches the current `build-base` release. I.e. ubuntu-16.04
@@ -51,6 +49,17 @@ parts:
       sudo -E apt-get build-dep -y ./
       ./get-deps.sh --skip-unused-check
     override-build: |
+      # unfortunately we do not have something like "snapcraftctl get-version"
+      # so we have to re-run mkversion.sh to check if the version number was set
+      # as "dirty" from the override-pull step
+      if sh -x ./mkversion.sh --output-only | grep "dirty"; then
+        mkdir -p $SNAPCRAFT_PART_INSTALL/usr/lib/snapd
+        (
+          echo "dirty git tree during build detected:"
+          git status
+          git diff
+        ) > $SNAPCRAFT_PART_INSTALL/usr/lib/snapd/dirty-git-tree-info.txt
+      fi
       # unset the LD_FLAGS and LD_LIBRARY_PATH vars that snapcraft sets for us
       # as those will point to the $SNAPCRAFT_STAGE which on re-builds will
       # contain things like libc and friends that confuse the debian package

…uilds

We don't want to fail dirty builds since this means that no local builds of the
snapd snap can be performed unless all changes are committed, which is
unfortunate and adds unnecessary steps for snapd developers during development.

Instead when we detect that a build is dirty, include the information inside a
file that is put into the snap that can be inspected later on. This will make
it easy to spot what changed during the build of a dirty snap if for some
reason we ever have those sneak by into the released channels of the published
snap.

Also use build-packages for build dependencies instead of manual apt install
commands.

These changes produce for this exact change without being committed the
following file inside the snap:

```
dirty git tree during build detected:
On branch feature/build-packages-instead-of-override-pull
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   build-aux/snap/snapcraft.yaml

Untracked files:
  (use "git add <file>..." to include in what will be committed)

        2.53.3-changelog.txt
        2.53.4-changelog.txt

no changes added to commit (use "git add" and/or "git commit -a")
diff --git a/build-aux/snap/snapcraft.yaml b/build-aux/snap/snapcraft.yaml
index cd800ce..3aec80f 100644

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
--- a/build-aux/snap/snapcraft.yaml
+++ b/build-aux/snap/snapcraft.yaml
@@ -30,16 +30,14 @@ parts:
     plugin: nil
     source: .
     build-snaps: [go/1.13/stable]
+    # these packages are needed to call mkversion.sh in override-pull, all other
+    # dependencies are installed using apt-get build-dep
+    build-packages:
+      - git
+      - dpkg-dev
     override-pull: |
       snapcraftctl pull
       # set version, this needs dpkg-parsechangelog (from dpkg-dev) and git
-      sudo -E apt-get install -y dpkg-dev git
-      if sh -x ./mkversion.sh --output-only | grep "dirty"; then
-          echo "dirty git tree during build detected:"
-          git status
-          git diff
-          exit 1
-      fi
       snapcraftctl set-version "$(./mkversion.sh --output-only)"
       # Ensure that ./debian/ packaging which we are about to use
       # matches the current `build-base` release. I.e. ubuntu-16.04
@@ -51,6 +49,17 @@ parts:
       sudo -E apt-get build-dep -y ./
       ./get-deps.sh --skip-unused-check
     override-build: |
+      # unfortunately we do not have something like "snapcraftctl get-version"
+      # so we have to re-run mkversion.sh to check if the version number was set
+      # as "dirty" from the override-pull step
+      if sh -x ./mkversion.sh --output-only | grep "dirty"; then
+        mkdir -p $SNAPCRAFT_PART_INSTALL/usr/lib/snapd
+        (
+          echo "dirty git tree during build detected:"
+          git status
+          git diff
+        ) > $SNAPCRAFT_PART_INSTALL/usr/lib/snapd/dirty-git-tree-info.txt
+      fi
       # unset the LD_FLAGS and LD_LIBRARY_PATH vars that snapcraft sets for us
       # as those will point to the $SNAPCRAFT_STAGE which on re-builds will
       # contain things like libc and friends that confuse the debian package
```
…apd snaps

This should prevent committing changes which create dirty snapd snap versions
in PRs.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
@codecov-commenter
Copy link

Codecov Report

Merging #11197 (1a3cd2e) into master (d820b8c) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11197   +/-   ##
=======================================
  Coverage   78.36%   78.36%           
=======================================
  Files         922      922           
  Lines      105128   105128           
=======================================
+ Hits        82381    82385    +4     
+ Misses      17614    17611    -3     
+ Partials     5133     5132    -1     
Flag Coverage Δ
unittests 78.36% <ø> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
overlord/ifacestate/helpers.go 77.45% <0.00%> (+0.48%) ⬆️

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 d820b8c...1a3cd2e. Read the comment docs.

build-aux/snap/snapcraft.yaml Show resolved Hide resolved
This is already planned soonish, so this should become a TODO to use the new
"craftctl" command for this when that is ready/available.

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

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you!

@mvo5 mvo5 merged commit 7fcec7b into snapcore:master Jan 5, 2022
Copy link
Contributor

@MiguelPires MiguelPires left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@anonymouse64 anonymouse64 deleted the feature/build-packages-instead-of-override-pull branch January 5, 2022 16:50
@anonymouse64 anonymouse64 added this to the 2.54 milestone Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants