snapcraft should use command-chain for hooks

Bug #1824255 reported by Ian Johnson
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Snapcraft
Status tracked in Trunk
Legacy
Undecided
Kyle Fazzari
Trunk
Undecided
Chris Patterson
snapd
Undecided
Unassigned

Bug Description

During snap install, we commonly need to configure a generic config file that has invalid or dummy values built into the snap with runtime files and sometimes we need to use tools available in the core snap (i.e. sed, grep, awk) to do this such as jq, so we stage those tools into the snap.

In normal snapcraft.yaml defined apps, the environment variables get setup such that we can just use "jq" in a shell script and it is on the $PATH with $LD_LIBRARY_PATH set appropriately, but we need to use jq from a hook and the environment variables aren't setup at all for hooks. This may be by design but is unfortunate as now we have code like this:

```
# add things from the snap's $PATH to here
export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin:$PATH"

# setup LD_LIBRARY_PATH, we need to handle the different architecture paths -
# this snippet is copied out of one of the generated snapcraft wrappers
case $(arch) in
    x86_64)
        MULTI_ARCH_PATH="x86_64-linux-gnu";;
    arm*)
        MULTI_ARCH_PATH="arm-linux-gnueabihf";;
    aarch64)
        MULTI_ARCH_PATH="aarch64-linux-gnu";;
    *)
        echo "architecture $ARCH not supported"
        exit 1
        ;;
esac

# Note - the env var SNAP_LIBRARY_PATH is set by snapd for hooks, so it can be referenced here
export LD_LIBRARY_PATH="$SNAP_LIBRARY_PATH:$LD_LIBRARY_PATH:$SNAP/lib:$SNAP/usr/lib:$SNAP/lib/$MULTI_ARCH_PATH:$SNAP/usr/lib/$MULTI_ARCH_PATH"
```

It would be great if snapcraft or snapd could somehow facilitate adding generic boilerplate code like this for hooks. One easy way I think would be to have snapd gain support for command-chain for hooks like it does for apps today and then in the snap.yaml it would have a snapcraft generated hook that runs before the actual hook in the snap.

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

In snapcraft, if you create the hook through a part, command-chain will be properly setup with the right environment, the hooks need logic to install to $SNAPCRAFT_PART_INSTALL/snap/hooks/<hook-name>

The case you are presenting is probably the case where you just add hooks to <project_root>/snap/hooks which get straight up copied into <snap_root>/meta/hooks. This can be solved by setting a global environment.

The fact that the environment keyword at the root of a project is not respected was a snapd issue that has since been solved https://github.com/snapcore/snapd/pull/5498

I am marking the snapcraft issue as Invalid for such reason, and will let the snapd team attest to what I wrote and triage appropriately.

Changed in snapcraft:
status: New → Invalid
Revision history for this message
Ian Johnson (anonymouse67) wrote :

Can you give an example of creating hooks through a snapcraft part? Would we just copy hooks from somewhere in the git tree to $SNAPCRAFT_PART_INSTALL/meta/hooks/intstall, etc. ?

Also using environment at a global level doesn't solve the issue for snaps like this because the environment variables we need are not static, as shown in the snippet above they are runtime dependent.

Revision history for this message
Ian Johnson (anonymouse67) wrote :

Actually I see from your comment it's snap/hooks not meta/hooks...

Revision history for this message
Sergio Schvezov (sergiusens) wrote :
Changed in snapcraft:
status: Invalid → New
Revision history for this message
Ian Johnson (anonymouse67) wrote :

Unfortunately, even after generating the hooks using a part in my snapcraft.yaml I can't get this to work, as the generated snap.yaml never specifies a command-chain for the hooks. I have tried also specifically mentioning the hooks in the top-level snapcraft.yaml with:

```yaml
hooks:
  configure:
    plugs:
      - network
```

Do I also need to declare something else in the snapcraft.yaml to get the command-chain to be generated for the hooks?

Revision history for this message
Kyle Fazzari (kyrofa) wrote :

Confirmed, while a wrapper is still generated for hooks, wrappers don't include environment anymore-- that's all command chain, and hooks aren't using it like apps are.

Changed in snapcraft:
status: New → Confirmed
Revision history for this message
Ian Johnson (anonymouse67) wrote :

As a work-around for this, if you define an app in snapcraft.yaml to use the full adapter, snapcraft still generates a command-chain that is usable for hooks that you can pass along to the snap.yaml using passthrough in the snapcraft.yaml like so:

```

assumes: [command-chain]
passthrough:
  hooks:
    configure:
      command-chain:
      - snap/command-chain/snapcraft-runner

apps:
  mything:
    adapter: full
...
```

Revision history for this message
Kyle Fazzari (kyrofa) wrote :

I see two ways to support this. Either snapcraft writes all hooks into the snap.yaml and uses snapd's `command-chain` directive and add the `assumes` (like the `full` adapter does for apps), or exec the command chain in the wrapper created for the hook.

Revision history for this message
Kyle Fazzari (kyrofa) wrote :

I'd like to see us move to the `full` adapter by default, in which case using snapd's `command-chain` for hooks would be best. However, I think that ship has sailed until the next base is introduced given the backward-incompatible change to apps. So my recommendation is to just exec the generated chain in the hook's wrapper, keeping current behavior.

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

I don't believe this is something that requires changes to snapd itself. The format supports all kinds of hooks and the way they are executed does not need to change. I think this is a snapcraft-side feature to make it easier to define hooks.

Please reopen the snapd task if you disagree.

Changed in snapd:
status: New → Invalid
summary: - snapcraft+snapd should allow generating/using wrappers for hooks
+ snapcraft should use command-chain for hooks
Revision history for this message
Ian Johnson (anonymouse67) wrote :

This is still a problem for snapcraft hooks, because even when I have a part that stages the hooks into $SNAPCRAFT_PART_INSTALL/snap/hooks/<hook-name> the generated hook in $SNAPCRAFT_PRIME/meta/hooks/<hook-name> just looks like this:

```
#!/bin/sh
exec "$SNAP/snap/hooks/install" "$@"
```

and the generated snap.yaml doesn't specify the install hook at all, thus it also doesn't specify command-chain, so all snapcraft is doing in this situation is creating another shell script to call the first shell script.

This is with snapcraft edge:

```
$ snapcraft version
snapcraft, version 3.9.2+git139.g621eea18
$ snap info snapcraft | grep installed
installed: 3.9.2+git139.g621eea18 (3828) 62MB classic
```

Revision history for this message
Chris Patterson (cjp256) wrote :

One of my upcoming tasks is to use command-chain for hooks in core20+.

I don't know much about hooks, but it's unclear to me from the documentation whether the mere presence of the hook file is sufficient, or if it also needs a corresponding entry in the snap[craft].yaml.

It _appears_ to me that a corresponding entry for snap[craft].yaml is required only if you have extra configuration parameters required for the hook (e.g. plugs). But I could be wrong..? Should snap.yaml have entries for _all_ configured/installed hooks?

Revision history for this message
Ian Johnson (anonymouse67) wrote :

Chris, if the snap.yaml does not have an entry for the hook, it is known as an implicit hook, and it should basically operate the same way as an explicitly declared hook in the snap.yaml without any plugs/environment set (but there have been bugs about this before). So it is not necessary to explicitly declare the hook in the snap.yaml or the snapcraft.yaml for snapd to use it at runtime.

> It _appears_ to me that a corresponding entry for snap[craft].yaml is required only if you have extra configuration parameters required for the hook (e.g. plugs).

I think this understanding is correct today, but see below for how I think this should change.

IMHO the expected resolution to this bug is that:

1. snapcraft allows declaring command-chains for hooks in the snapcraft.yaml and this works the same as doing so in the snap.yaml, where if you specify adapter: full, then the command-chain specified in the snapcraft.yaml gets pre-pended with the snapcraft-runner
2. snapcraft auto-generates the command-chain in the snap.yaml with snapcraft-runner and the specified hook even when there is no explicit hooks section set in the snapcraft.yaml (I think it's reasonable to only do this behavior when the hook is generated by a snapcraft part and let implicit hooks operate the same way as they do today)

This may or may not break backwards compatibility, if it does then I think it's fine to wait until core20 (?) to make the full change. If that's the case, a short-term solution for my situation would be to include the snapcraft-runner in the generated hook today, i.e. to have the generated hook look something like:

```
#!/bin/sh
exec "$SNAP/snap/command-chain/snapcraft-runner" "$SNAP/snap/hooks/install" "$@"
```

Revision history for this message
Kyle Fazzari (kyrofa) wrote :

Note that this bug is a huge regression from snapcraft 2.x (i.e. snapcraft without bases) that hampers migration to using bases.

Revision history for this message
Kyle Fazzari (kyrofa) wrote :

Ooo, I lied, that regressed in legacy as well.

Revision history for this message
Kyle Fazzari (kyrofa) wrote :

Ah, it's more nuanced. The deb in Xenial (still the default for the LP snap builders) doesn't have this problem. It regressed in the snap, both with and without bases. This should fix it for without bases: https://github.com/snapcore/snapcraft/pull/2893

Revision history for this message
Chris Patterson (cjp256) wrote :

OK I think I finally get it now :) Thanks for educating me everyone.

I tested snapcraft 3.8, and it does not appear to regress on this front. So I searched further back to see what I think is the cause may be and stumbled on this commit which didn't appear in a snapcraft-legacy release until "2.44" (Kyle pointed out Xenial in unaffected, at 2.43.1):
https://github.com/snapcore/snapcraft/pull/2257/commits/7cba5de7ce52c93440c3711c8386fee63d65f455

In that commit, the scope of the common.env that is set to snap_env is reduced from all of create_snap_packaging() to write_snap_yaml(). Prior to that, the behavior looks like it probably generated the correct environment for snapcraft-generated trampolines (meta/hooks->snap/hooks). If that's the failure, then I don't think it's ever actually worked in 3.x.

I talked to Sergio earlier and he pressed for the use of command-chain, which seems like a good idea. In this case, we'll just be inserting snapcraft-runner into the command-chain. The question is for what scope:

(1) hooks using snapcraft-generated trampolines (closest to old behavior?)
(2) all hooks (new functionality?)

@kyrofa suggested that he would like to see it done for all hooks, and had a good idea above about gating compatibility changes on core20. I think if we choose to apply command-chain to all hooks (#2), the safest choice will be to do it for core20 onwards.

In the meantime, we can possibly automatically re-enable this for #1 unless that boat has sailed? Publishers of strict core/core18 snaps that bypass the trampoline (using pre-installed <project>/snap/hooks/...) can safely opt-in to this behavior by adding their own command-chain ["snap/command-chain/snapcraft-runner"], I can PR a change to ensure it's available if found in user-supplied command-chain.

In summary, the proposal is:

(1) include environment in trampolines generated by snapcraft:

  (a) master: insert snapcraft-runner into command-chain only for these hooks & ensure snapcraft-runner is generated if user-configured.

  (b) legacy: use Kyle's PR (2893)

(2) add new PR for core20 to include snapcraft-runer in command-chain unconditionally for all hooks

Thoughts?

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

While there seems to be a regression, the bug here is explicitly about using command-chain for hooks and not the regression in wrapper generation we have.

I am, and have been +1, on automatically setting snapcraft's command-chain for hooks for core20 and on but only for what seems to be called "trampolined" hooks.

hooks coming from "snap/hooks" should be as vanilla as they are, anyone writing hooks that way knows what they are doing and can set "environment" and "command-chain" in snapcraft.yaml (and consequently snap.yaml)

Revision history for this message
Kyle Fazzari (kyrofa) wrote :

> the bug here is explicitly about using command-chain for hooks and not the regression in wrapper generation we have.

Well, not really. It's about hooks not having the same environment as apps. That can be solved in one of two ways: fixing the regression, or using command-chain. If we want to go the command-chain route, no problem.

Revision history for this message
Kyle Fazzari (kyrofa) wrote :

Although it appears that, given https://github.com/snapcore/snapcraft/pull/2897 was merged. we're not going the command-chain route for master, which is also cool. I can confirm that hook wrappers in edge now get the environment placed within them. Question is, should that be Fix Committed for this bug, or would you like to track the hook command-chain work here instead (which needs a bit of unrelated work)?

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

Kyle, command-chain for hooks if they come from a part is fine.

This bug is about automatically setting a command-chain for a hook dropped in the project tree at

snap/hooks

for which we do not generate wrappers for either as it is meant to be verbatim.

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

To avoid further confusion, this bug is asking for adding environment for what is the `install` hook in my example (through command-chain), but if that is really wanted, just put it in a as seen below

```
sergiusens@gotham:~/projects/snaps/h$ find
.
./refresh
./snap
./snap/snapcraft.yaml
./snap/hooks
./snap/hooks/install
sergiusens@gotham:~/projects/snaps/h$ cat snap/snapcraft.yaml
name: my-snap-name # you probably want to 'snapcraft register <name>'
version: '0.1' # just for humans, typically '1.2+git' or '1.3.2'
summary: Single-line elevator pitch for your amazing snap # 79 char long summary
description: |
  This is my-snap's description. You have a paragraph or two to tell the
  most important story about your snap. Keep it under 100 words though,
  we live in tweetspace and your description wants to look good in the snap
  store.

grade: devel # must be 'stable' to release into candidate/stable channels
confinement: devmode # use 'strict' once you have the right plugs and slots

parts:
  my-part:
    # See 'snapcraft plugins'
    plugin: dump
    source: .
    organize:
      refresh: snap/hooks/refresh
sergiusens@gotham:~/projects/snaps/h$ cat refresh
sergiusens@gotham:~/projects/snaps/h$ cat snap/hooks/install
echo "NO WRAPPERS FOR ME"
sergiusens@gotham:~/projects/snaps/h$ snapcraft prime
This snapcraft project does not specify the base keyword, explicitly setting the base keyword enables the latest snapcraft features.
This project is best built on 'Ubuntu 16.04', but is building on a 'Ubuntu 20.04' host.
Read more about bases at https://docs.snapcraft.io/t/base-snaps/11198
Pulling my-part
Building my-part
Staging my-part
Priming my-part
sergiusens@gotham:~/projects/snaps/h$ cat prime/meta/hooks/refresh
#!/bin/sh
export PATH="$SNAP/usr/sbin:$SNAP/usr/bin:$SNAP/sbin:$SNAP/bin:$PATH"
export LD_LIBRARY_PATH=$SNAP_LIBRARY_PATH:$LD_LIBRARY_PATH
exec "$SNAP/snap/hooks/refresh" "$@"
sergiusens@gotham:~/projects/snaps/h$ cat prime/meta/hooks/install
echo "NO WRAPPERS FOR ME"
```

Revision history for this message
Ian Johnson (anonymouse67) wrote :

Sorry if I filed this bug poorly, but the original problem I had with this bug is that no hooks had environment setup for them, whether they were trampoline style hooks generated by a part or not. That's the bug I care about, it's fine to have hooks generated by a part for me, I just want them to have the same environment as apps.

I have no opinion on whether or not snaps should use command-chain for this or generate trampoline shell scripts, etc. I just thought that command-chain would be an easy way to do this.

I also have no (strong) opinion on what the behavior should be for hooks that are placed directly in snap/hooks (i.e. are not trampoline style hooks).

Really to me, the issue was that using a modern snapcraft as a snap, I had no way to tell snapcraft that I want my hooks to have their environment setup.

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

OK then, this should be Fix Committed.

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

Part of 3.10

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers