project_loader: add basic template support #2189

kyrofa · 2018-08-02T22:44:11Z

Have you followed the guidelines for contributing?
Have you signed the CLA?
If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
If this is a new feature. Have you discussed the design on the forum?
Have you successfully run ./runtests.sh static?
Have you successfully run ./runtests.sh unit?

This PR resolves LP: #1785109 by adding support for templates. Templates have the ability to add YAML snippets to apps, YAML snippets to parts, and indeed add entire parts to a project. Don't introduce any actual templates, just the groundwork for them.

In a desire to reduce duplication between snaps, introduce support for templates. Templates have the ability to add YAML snippets to apps, YAML snippets to parts, and indeed add entire parts to a project. Don't introduce any actual templates, just the groundwork for them. LP: #1785109 Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

codecov-io · 2018-08-03T01:44:58Z

Codecov Report

Merging #2189 into master will decrease coverage by <.01%.
The diff coverage is 91.25%.

@@            Coverage Diff             @@
##           master    #2189      +/-   ##
==========================================
- Coverage   91.21%   91.21%   -0.01%     
==========================================
  Files         208      210       +2     
  Lines       13160    13318     +158     
  Branches     1957     1982      +25     
==========================================
+ Hits        12004    12148     +144     
- Misses        788      795       +7     
- Partials      368      375       +7

Impacted Files	Coverage Δ
snapcraft/internal/project_loader/_env.py	`96.22% <ø> (ø)`	⬆️
snapcraft/internal/project_loader/errors.py	`96.15% <100%> (+0.59%)`	⬆️
snapcraft/internal/project_loader/_schema.py	`100% <100%> (ø)`	⬆️
snapcraft/internal/project_loader/_config.py	`99.52% <100%> (ø)`	⬆️
snapcraft/cli/_runner.py	`95.45% <100%> (+0.1%)`	⬆️
snapcraft/internal/common.py	`97.22% <100%> (+0.12%)`	⬆️
snapcraft/internal/project_loader/__init__.py	`92.3% <100%> (+0.3%)`	⬆️
snapcraft/internal/dirs.py	`53.33% <50%> (-0.52%)`	⬇️
snapcraft/cli/templates.py	`87.09% <87.09%> (ø)`
snapcraft/internal/project_loader/_templates.py	`91.17% <91.17%> (ø)`
... and 2 more

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 b772997...648a67b. Read the comment docs.

sergiusens · 2018-08-09T20:14:40Z

setup.py

@@ -148,6 +149,10 @@
                "share/snapcraft/libraries",
                ["libraries/" + x for x in os.listdir("libraries")],
            ),
+            (
+                "share/snapcraft/template-data",
+                ["templates/" + x for x in os.listdir("templates")],


I wanted to get rid of this mechanism and do something a bit more pkg_resource driven as this fails when creating a source package to push over to pypi (unless you edit that other file https://github.com/snapcore/snapcraft/blob/master/MANIFEST.in). If possible, we should get away from that early.

It's probably beyond the scope of this PR to change this, so I'll edit the manifest here, but yeah, let's change that.

sergiusens · 2018-08-09T20:15:53Z

snapcraft/cli/templates.py

+
+    template_names = os.listdir(common.get_templatesdir())
+    if not template_names:
+        click.echo("No templates supported")


maybe even tie it to the base? So then different people running it don't get confused with, hey I get some, what about you? Or is this going to list everything regardless of base?

sergiusens · 2018-08-09T20:16:09Z

snapcraft/cli/templates.py

+    # Wrap the output depending on terminal size
+    width = common.get_terminal_width()
+    for line in common.format_output_in_columns(
+        sorted(template_names), max_width=width


alternatively we should list what bases it supports here

I agree, let me try some different formatting, here.

sergiusens · 2018-08-09T20:17:57Z

snapcraft/internal/project_loader/_templates.py

+    # Don't modify the dict passed in
+    yaml_data = copy.deepcopy(yaml_data)
+
+    # Get the base being used for this project (defaults to "core")


core is not a base :-)
I would default to nothing and if needed, introduce this later

IOW, let's tie this to the use of bases.

Remember that today, you can run snapcraft on arch and if no stage packages or build-packages are introduced, chances are, it might just work. This would introduce another vector of confusion.

It would be easy to raise an exception here, but that's an artificial constraint. I disagree on this point, for a few reasons:

Remote parts are a problem today, with a ton of assumed knowledge and documentation issues. They are hard to use, and templates will be better. They are also very widely used (desktop helpers in particular).

Templates are the only option for folks using remote parts that move to bases (as remote parts are not supported). By not allowing folks to transition to templates before they transition to using bases, we make the move to using bases quite a lot more work than is actually necessary.

Running snapcraft today on things other than Ubuntu is not something we support without bases. Making decisions based on how snapcraft runs on other distros today sounds a bit like support 😉 .

Unless supporting templates before bases actually makes our lives harder, I think it's a good idea. We can chat about this more in-person, though.

We chatted about this in person, and the big concern is simply that the core16 base, once it exists, cannot guarantee that it will continue building on trusty as well, which supporting templates without bases would entail (i.e. defaulting to core16 templates). As a result, I've started raising an exception if no base is specified. Basically, supporting templates before bases will make our lives harder.

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

…plates

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

sergiusens · 2018-08-10T17:44:29Z

templates/README.md

+
+```yaml
+# Keys in the root of the yaml specify the supported bases
+core16:


change this to core18, it is the new and flashy (and currently existing) base.

sorry, I missed a chunk

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

…plates

sergiusens · 2018-08-14T14:43:15Z

snapcraft/internal/project_loader/_templates.py

@@ -121,6 +116,10 @@ def load_template(template_name: str) -> Dict[str, Any]:


 def _find_template(base: str, template_name: str) -> Dict[str, Any]:
+    # A bade is required in order to use templates, so raise an error if not specified.


Well duh. What would YOU call it? 😉

Fixed.

sergiusens · 2018-08-14T14:48:24Z

templates/README.md

+      # Yaml snippet you want to add to parts in the `snapcraft.yaml` (e.g.
+      # `after`)
+
+    # You can also put whatever parts here you want, and they will be put


whatever parts you want here?

Yikes, awkward wording indeed. Fixed.

sergiusens · 2018-08-14T14:49:47Z

templates/README.md

+    # containing this README.
+    my-template-part:
+      plugin: dump
+      source: $SNAPCRAFT_TEMPLATES_DIR/my-template-name/src


ok, this might be the reason. Maybe this should be more encapsulated to the actual template instead?

Well, remember that templates can only alter the snapcraft.yaml. They're not particularly special beasts; we can't really do any magic there or we kill the ability for someone to copy/paste them into their local snapcraft.yaml to tweak.

oh, I mean SNAPCRAFT_TEMPLATE_DIR specific to the template and not the entire namespace

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

…plates

sergiusens

the only thing that worried me a bit is the use of an overreaching environment variable. Given that everything will remain under tight control, the fact that this environment is public is not a worry.

kyrofa · 2018-08-14T19:32:42Z

ftp.gnu.org is down right now, which caused the autotools spread tests to fail, but I just tested them locally, so I'm merging.

sergiusens reviewed Aug 9, 2018

View reviewed changes

kyrofa added 4 commits August 10, 2018 07:52

Use core16, not core

16acf1b

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

Add templates to manifest

4966956

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

Merge remote-tracking branch 'origin/master' into feature/1785109/tem…

511f8ca

…plates

Fix template readme

aee3aea

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa force-pushed the feature/1785109/templates branch from 1426773 to 21dd614 Compare August 10, 2018 16:30

Tie templates to bases

b23a380

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

kyrofa force-pushed the feature/1785109/templates branch from 21dd614 to b23a380 Compare August 10, 2018 16:32

sergiusens reviewed Aug 10, 2018

View reviewed changes

sergiusens previously approved these changes Aug 10, 2018

View reviewed changes

kyrofa added 4 commits August 10, 2018 11:59

Change readme to use core18

41b0aac

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

Better base check

282f0c6

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

Use ordered dict to ensure consistent print

ecd0519

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

Merge remote-tracking branch 'origin/master' into feature/1785109/tem…

efab76c

…plates

sergiusens reviewed Aug 14, 2018

View reviewed changes

kyrofa added 2 commits August 14, 2018 07:57

Spelling/grammar fixes

0c7abf9

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

Merge remote-tracking branch 'origin/master' into feature/1785109/tem…

648a67b

…plates

kyrofa force-pushed the feature/1785109/templates branch from a1940ae to 648a67b Compare August 14, 2018 14:59

sergiusens approved these changes Aug 14, 2018

View reviewed changes

kyrofa merged commit efcf047 into canonical:master Aug 14, 2018

kyrofa deleted the feature/1785109/templates branch August 14, 2018 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

project_loader: add basic template support #2189

project_loader: add basic template support #2189

kyrofa commented Aug 2, 2018 •

edited

codecov-io commented Aug 3, 2018 •

edited

sergiusens Aug 9, 2018

kyrofa Aug 10, 2018

sergiusens Aug 9, 2018

sergiusens Aug 9, 2018

kyrofa Aug 10, 2018

sergiusens Aug 9, 2018

sergiusens Aug 9, 2018

sergiusens Aug 9, 2018

kyrofa Aug 10, 2018 •

edited

kyrofa Aug 10, 2018 •

edited

sergiusens Aug 10, 2018

sergiusens Aug 14, 2018

kyrofa Aug 14, 2018

sergiusens Aug 14, 2018

kyrofa Aug 14, 2018

sergiusens Aug 14, 2018

kyrofa Aug 14, 2018 •

edited

sergiusens Aug 14, 2018

sergiusens left a comment •

edited

kyrofa commented Aug 14, 2018

		@@ -121,6 +116,10 @@ def load_template(template_name: str) -> Dict[str, Any]:


		def _find_template(base: str, template_name: str) -> Dict[str, Any]:
		# A bade is required in order to use templates, so raise an error if not specified.

project_loader: add basic template support #2189

project_loader: add basic template support #2189

Conversation

kyrofa commented Aug 2, 2018 • edited

codecov-io commented Aug 3, 2018 • edited

Codecov Report

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

Choose a reason for hiding this comment

kyrofa Aug 10, 2018 • edited

Choose a reason for hiding this comment

kyrofa Aug 10, 2018 • 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 Aug 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiusens left a comment • edited

Choose a reason for hiding this comment

kyrofa commented Aug 14, 2018

kyrofa commented Aug 2, 2018 •

edited

codecov-io commented Aug 3, 2018 •

edited

kyrofa Aug 10, 2018 •

edited

kyrofa Aug 10, 2018 •

edited

kyrofa Aug 14, 2018 •

edited

sergiusens left a comment •

edited