-
Notifications
You must be signed in to change notification settings - Fork 441
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
project_loader: add basic template support #2189
Conversation
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 Report
@@ 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
Continue to review full report at Codecov.
|
@@ -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")], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
template_names = os.listdir(common.get_templatesdir()) | ||
if not template_names: | ||
click.echo("No templates supported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
snapcraft/cli/templates.py
Outdated
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively we should list what bases it supports here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let me try some different formatting, here.
# Don't modify the dict passed in | ||
yaml_data = copy.deepcopy(yaml_data) | ||
|
||
# Get the base being used for this project (defaults to "core") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core is not a base :-)
I would default to nothing and if needed, introduce this later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOW, let's tie this to the use of bases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
1426773
to
21dd614
Compare
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
21dd614
to
b23a380
Compare
templates/README.md
Outdated
|
||
```yaml | ||
# Keys in the root of the yaml specify the supported bases | ||
core16: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change this to core18, it is the new and flashy (and currently existing) base.
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well duh. What would YOU call it? 😉
Fixed.
templates/README.md
Outdated
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whatever parts you want here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, awkward wording indeed. Fixed.
# containing this README. | ||
my-template-part: | ||
plugin: dump | ||
source: $SNAPCRAFT_TEMPLATES_DIR/my-template-name/src |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this might be the reason. Maybe this should be more encapsulated to the actual template instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I mean SNAPCRAFT_TEMPLATE_DIR specific to the template and not the entire namespace
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
a1940ae
to
648a67b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
./runtests.sh static
?./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.