Add support for hooks. #1026

kyrofa · 2017-01-05T01:35:04Z

Hooks are now supported in snapd and the click reviewer tools. Time to finish off LP: #1586465 by supporting them in snapcraft as well. This is done by convention, and two methods are supported:

Place hook executables in the snap/hooks/ directory, and they will be copied into the snap automatically.
Install hook executables from parts into the snap/hooks/ directory.

Hooks are now supported in snapd and the click reviewer tools. Time to support them in snapcraft as well. This is done by convention, and two methods are supported: 1. Place hook executables in the snap/hooks/ directory, and they will be copied into the snap automatically. 2. Install hook executables from parts into the snap/hooks/ directory. LP: #1586465 Signed-off-by: Kyle Fazzari <kyle@canonical.com>

codecov-io · 2017-01-05T02:03:46Z

Current coverage is 96.34% (diff: 98.90%)

Merging #1026 into master will increase coverage by 0.01%

@@             master      #1026   diff @@
==========================================
  Files           194        194          
  Lines         17070      17161    +91   
  Methods           0          0          
  Messages          0          0          
  Branches       1307       1320    +13   
==========================================
+ Hits          16443      16534    +91   
  Misses          426        426          
  Partials        201        201

Powered by Codecov. Last update aec853a...6cf22a0

sergiusens

Looks pretty good! Just a few comments

sergiusens · 2017-01-05T13:48:46Z

snapcraft/internal/meta.py

@@ -58,7 +60,7 @@ class CommandError(Exception):

 def create_snap_packaging(config_data, snap_dir, parts_dir):
    """Create snap.yaml and related assets in meta.
-    Create  the meta directory and provision it with snap.yaml
+    Create the meta directory and provision it with snap.yaml
    in the snap dir using information from config_data.


since you edited this text you can add the part about checking for snap

sergiusens · 2017-01-05T13:53:39Z

snapcraft/internal/meta.py

+                file_utils.link_or_copy(source, destination)
+
+        # Now write hook wrappers for any hooks included in that directory.
+        self._generate_hook_wrappers()


I would only generate hook wrappers for things coming from parts just to not break any assumption a manually crafted hook would have.
To do so, I guess it is just enough to rename _general_hook_wrappers to a public one and call it before packaging.write_snap_directory() above.

It makes sense to move it out as well as write_snap_directory will eventually do more.

I would only generate hook wrappers for things coming from parts just to not break any assumption a manually crafted hook would have.

If we don't generate wrappers for the manually crafted ones, there's nothing connecting the dots between our magic location (snap/hooks) and snapd's (meta/hooks). Are you thinking we'd just generate a dumb wrapper?

I'm trying to think of assumptions a manually crafted hook might have that would break with our setting environment variables... I'm not coming up with any, do you know? However, I can quickly come up with a breakage if we don't set them: if the manually crafted hook uses any python executables within it, it'll immediately break. The same applies to any ROS tools, and so on. And there's no way around it except to move them out of the snap directory and install them via a part. The inconsistency there might make for a poor experience.

An example, a hook wanting to use the system python where the wrapper gets in the way.

How would that be done in an app?

If I have this tree:

./ ./snap/hooks/configure

After snapping I expect this configure to land as is on

./prime/ meta/hooks/configure snap/hooks/configure

If in the other case this came from a part, those two configure's would be different, the former the wrapped one while the latter the built one.

We don't have apps by convention

Apps are related in that they're runnable things exposed by snapcraft, as are hooks. One is run by the user, one is run by snapd, but they're both exposed by snapcraft. What I mean to say is, if we don't expect developers to want to do such a thing in apps, why do we expect them to want to do such a thing in a hook?

we don't have apps byt convention now, do we? We were also given the guidelines to not compare these to apps ;-)

This is sitting right next to the code that is doing the same job for apps. It's in the same file, a few lines away! It's difficult not to compare how the two are handled. That said, I will make this change.

Alright, done.

Only wrap the ones provided by parts. Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

This requires most recent snapcraft with support for hooks. canonical/snapcraft#1026

Hooks are now supported in snapd and the click reviewer tools. Time to support them in snapcraft as well. This is done by convention, and two methods are supported: 1. Place hook executables in the snap/hooks/ directory, and they will be copied into the snap automatically. 2. Install hook executables from parts into the snap/hooks/ directory. LP: #1586465 Signed-off-by: Kyle Fazzari <kyle@canonical.com>

sergiusens requested changes Jan 5, 2017

View reviewed changes

kyrofa added 3 commits January 5, 2017 11:17

Don't wrap hooks provided in snap/hooks.

85015c2

Only wrap the ones provided by parts. Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Fix integration test.

f9478b6

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Update docs about snap directory.

6cf22a0

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

sergiusens approved these changes Jan 5, 2017

View reviewed changes

sergiusens merged commit 01880ee into canonical:master Jan 5, 2017

longsleep added a commit to longsleep/build-pine64-image that referenced this pull request Jan 9, 2017

Build gadget snap with snapcraft

267ad78

This requires most recent snapcraft with support for hooks. canonical/snapcraft#1026

longsleep added a commit to longsleep/build-pine64-image that referenced this pull request Jan 9, 2017

Build gadget snap with snapcraft

5c8af02

This requires most recent snapcraft with support for hooks. canonical/snapcraft#1026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for hooks. #1026

Add support for hooks. #1026

kyrofa commented Jan 5, 2017 •

edited

codecov-io commented Jan 5, 2017 •

edited

sergiusens left a comment

sergiusens Jan 5, 2017

kyrofa Jan 5, 2017

sergiusens Jan 5, 2017

kyrofa Jan 5, 2017 •

edited

sergiusens Jan 5, 2017

kyrofa Jan 5, 2017

sergiusens Jan 5, 2017

sergiusens Jan 5, 2017

kyrofa Jan 5, 2017

sergiusens Jan 5, 2017

kyrofa Jan 5, 2017 •

edited

kyrofa Jan 5, 2017

Add support for hooks. #1026

Add support for hooks. #1026

Conversation

kyrofa commented Jan 5, 2017 • edited

codecov-io commented Jan 5, 2017 • edited

Current coverage is 96.34% (diff: 98.90%)

sergiusens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Jan 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Jan 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa commented Jan 5, 2017 •

edited

codecov-io commented Jan 5, 2017 •

edited

kyrofa Jan 5, 2017 •

edited

kyrofa Jan 5, 2017 •

edited