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

refactor: use init command from craft-application #5129

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
include schema/*
include extensions/*
recursive-include snapcraft/templates *
6 changes: 3 additions & 3 deletions requirements-devel.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ click==8.1.7
codespell==2.3.0
colorama==0.4.6
coverage==7.6.1
craft-application==4.2.6
git+https://github.com/canonical/craft-application@work/CRAFT-3543-use-app-commands#egg=craft-application
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder to update this

craft-archives==2.0.0
craft-cli==2.7.0
craft-cli==2.9.0
craft-grammar==2.0.1
craft-parts==2.1.2
craft-platforms==0.4.0
Expand Down Expand Up @@ -89,7 +89,7 @@ pbr==6.0.0
pexpect==4.9.0
plaster==1.1.2
plaster-pastedeploy==1.0.1
platformdirs==4.2.2
platformdirs==4.3.6
pluggy==1.5.0
polib==1.2.0
progressbar==2.5
Expand Down
6 changes: 3 additions & 3 deletions requirements-docs.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ chardet==5.2.0
charset-normalizer==3.3.2
click==8.1.7
colorama==0.4.6
craft-application==4.2.6
git+https://github.com/canonical/craft-application@work/CRAFT-3543-use-app-commands#egg=craft-application
craft-archives==2.0.0
craft-cli==2.7.0
craft-cli==2.9.0
craft-grammar==2.0.1
craft-parts==2.1.2
craft-platforms==0.4.0
Expand Down Expand Up @@ -69,7 +69,7 @@ natsort==8.4.0
oauthlib==3.2.2
overrides==7.7.0
packaging==24.1
platformdirs==4.2.2
platformdirs==4.3.6
polib==1.2.0
progressbar==2.5
protobuf==5.27.3
Expand Down
6 changes: 3 additions & 3 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ cffi==1.17.1
chardet==5.2.0
charset-normalizer==3.3.2
click==8.1.7
craft-application==4.2.6
git+https://github.com/canonical/craft-application@work/CRAFT-3543-use-app-commands#egg=craft-application
craft-archives==2.0.0
craft-cli==2.7.0
craft-cli==2.9.0
craft-grammar==2.0.1
craft-parts==2.1.2
craft-platforms==0.4.0
Expand Down Expand Up @@ -37,7 +37,7 @@ mypy-extensions==1.0.0
oauthlib==3.2.2
overrides==7.7.0
packaging==24.1
platformdirs==4.2.2
platformdirs==4.3.6
progressbar==2.5
protobuf==5.27.3
psutil==6.0.0
Expand Down
8 changes: 6 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ def recursive_data_files(directory, install_directory):
"attrs",
"catkin-pkg; sys_platform == 'linux'",
"click",
"craft-application>=4.2.6,<5.0.0",
"craft-application @ git+https://github.com/canonical/craft-application@work/CRAFT-3543-use-app-commands#egg=craft-application",
"craft-archives~=2.0",
"craft-cli~=2.6",
"craft-cli~=2.9",
"craft-grammar>=2.0.1,<3.0.0",
"craft-parts>=2.1.2,<3.0.0",
"craft-platforms~=0.4",
Expand Down Expand Up @@ -174,6 +174,10 @@ def recursive_data_files(directory, install_directory):
+ recursive_data_files("keyrings", "share/snapcraft")
+ recursive_data_files("extensions", "share/snapcraft")
),
include_package_data=True,
package_data={
"snapcraft": ["templates/*"],
},
python_requires=">=3.10",
install_requires=install_requires,
extras_require=extras_requires,
Expand Down
12 changes: 1 addition & 11 deletions snapcraft/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,16 +150,6 @@ def _configure_services(self, provider_name: str | None) -> None:

super()._configure_services(provider_name)

@property
def command_groups(self):
"""Replace craft-application's LifecycleCommand group."""
_command_groups = super().command_groups
for index, command_group in enumerate(_command_groups):
if command_group.name == "Lifecycle":
_command_groups[index] = cli.CORE24_LIFECYCLE_COMMAND_GROUP

return _command_groups
Comment on lines -153 to -161
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would have had to do more hacks for InitCommand, hence canonical/craft-application#545.


@override
def _resolve_project_path(self, project_dir: pathlib.Path | None) -> pathlib.Path:
"""Overridden to handle the two possible locations for snapcraft.yaml."""
Expand Down Expand Up @@ -467,7 +457,7 @@ def create_app() -> Snapcraft:
services=snapcraft_services,
)

for group in cli.COMMAND_GROUPS:
for group in [cli.CORE24_LIFECYCLE_COMMAND_GROUP, *cli.COMMAND_GROUPS]:
app.add_command_group(group.name, group.commands)

return app
Expand Down
74 changes: 22 additions & 52 deletions snapcraft/commands/init.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,74 +16,44 @@

"""Snapcraft init command."""

from pathlib import Path
from textwrap import dedent
import argparse

from craft_application.commands import AppCommand
from craft_cli import emit
from overrides import overrides
import craft_application.commands
import craft_cli
from typing_extensions import override

from snapcraft import errors
from snapcraft.parts.yaml_utils import get_snap_project

_TEMPLATE_YAML = dedent(
"""\
name: my-snap-name # you probably want to 'snapcraft register <name>'
base: core24 # the base snap is the execution environment for this snap
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
class InitCommand(craft_application.commands.InitCommand):
"""Snapcraft init command."""

parts:
my-part:
# See 'snapcraft plugins'
plugin: nil
"""
)
@override
def run(self, parsed_args: argparse.Namespace) -> None:
"""Pack a directory or run the lifecycle and pack all artifacts."""
project_dir = self._get_project_dir(parsed_args)

if parsed_args.name:
craft_cli.emit.progress(
"Ignoring '--name' parameter because it is not supported yet.",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if this should be an error, a warning, or if I should change the default template to use this as the snap name.

Copy link
Contributor

Choose a reason for hiding this comment

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

if you changed the default template to support the name, would you be able to drop this class wholesale?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. The other extra things that snapcraft adds are:

  • checking for a project file under snapcraft.yaml, snap/snapcraft.yaml, build-aux/snap/snapcraft.yaml or .snapcraft.yaml
  • An emit message pointing to documentation.

I think the first is worth keeping. The second could be added as a comment to the snapcraft.yaml file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright. Then I think it should be an error and we should plan to address it soon

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for starting to support the name from the start, and address the rest in the separate item.

Out of curiosity, shouldn't you override the InitService and use the command from upstream (if you ignore the documentation message)?

permanent=True,
)

class InitCommand(AppCommand):
"""Initialize a snapcraft project."""

name = "init"
help_msg = "Initialize a snapcraft project."
overview = "Initialize a snapcraft project in the current directory."

@overrides
def run(self, parsed_args):
"""Initialize a snapcraft project in the current directory.

:raises SnapcraftError: If a snapcraft.yaml already exists.
"""
emit.progress("Checking for an existing 'snapcraft.yaml'.")

# if a project is found, then raise an error
try:
project = get_snap_project()
craft_cli.emit.progress("Checking for an existing 'snapcraft.yaml'.")
project = get_snap_project(project_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

this one sounds like maybe it should go on Snapcraft's (new) InitService
(we can also do this later)

raise errors.SnapcraftError(
"could not initialize a new snapcraft project because "
"could not initialise a new snapcraft project because "
f"{str(project.project_file)!r} already exists"
)
# the `ProjectMissing` error means a new project can be initialized
# the `ProjectMissing` error means a new project can be initialized
except errors.ProjectMissing:
emit.progress("Could not find an existing 'snapcraft.yaml'.")

snapcraft_yaml_path = Path("snap/snapcraft.yaml")

emit.progress(f"Creating {str(snapcraft_yaml_path)!r}.")
craft_cli.emit.progress("Could not find an existing 'snapcraft.yaml'.")

snapcraft_yaml_path.parent.mkdir(exist_ok=True)
snapcraft_yaml_path.write_text(_TEMPLATE_YAML, encoding="utf-8")
super().run(parsed_args)

emit.message(f"Created {str(snapcraft_yaml_path)!r}.")
emit.message(
craft_cli.emit.message(
Copy link
Contributor

Choose a reason for hiding this comment

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

it also feels like maybe this message should be on the initservice; or maybe a way for the service to give a template-based string to be presented to the user after init is done (this would be useful in rockcraft, which has per-profile docs)
(we can also do this later)

"Go to https://docs.snapcraft.io/the-snapcraft-format/8337 for more "
"information about the snapcraft.yaml format."
)
2 changes: 1 addition & 1 deletion snapcraft/extensions/env_injector.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def get_parts_snippet(self) -> Dict[str, Any]:

cargo build --target {toolchain} --release
mkdir -p $SNAPCRAFT_PART_INSTALL/bin/command-chain

cp target/{toolchain}/release/env-exporter $SNAPCRAFT_PART_INSTALL/bin/command-chain
""",
}
Expand Down
12 changes: 10 additions & 2 deletions snapcraft/parts/yaml_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,13 +218,21 @@ def apply_yaml(
return yaml_data


def get_snap_project() -> _SnapProject:
def get_snap_project(project_dir: Path | None = None) -> _SnapProject:
"""Find the snapcraft.yaml to load.

:param project_dir: The directory to search for the project yaml file. If not
provided, the current working directory is used.

:raises SnapcraftError: if the project yaml file cannot be found.
"""
for snap_project in _SNAP_PROJECT_FILES:
if snap_project.project_file.exists():
if project_dir:
snap_project_path = project_dir / snap_project.project_file
else:
snap_project_path = snap_project.project_file

if snap_project_path.exists():
return snap_project

raise errors.ProjectMissing()
Expand Down
17 changes: 17 additions & 0 deletions snapcraft/templates/simple/snap/snapcraft.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
name: my-snap-name # you probably want to 'snapcraft register <name>'
base: core24 # the base snap is the execution environment for this snap
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: nil
30 changes: 28 additions & 2 deletions tests/spread/general/init/task.yaml
Original file line number Diff line number Diff line change
@@ -1,14 +1,40 @@
summary: Run snapcraft init

environment:
PROFILE/default_profile: null
PROFILE/simple_profile: simple
PROFILE: null
PROJECT_DIR/default_dir: null
PROJECT_DIR/project_dir: test-project-dir
PROJECT_DIR: null
Comment on lines +4 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

I recently realized that in that case it will be one test case per variant, so it won't create a combination of simple_profile with project_dir set. Is that ok ?

It will create 4 variant's: default_profile, simple_profile, project_dir, default_dir.
In that case if I can see correctly default_profile and default_dir will be the same test with different name

Copy link
Contributor

Choose a reason for hiding this comment

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

spread variants are so annoying. I also only semi-recently internalized that this:

environment:
    VAR1/bla: var1-bla
    VAR1/ble: var1-ble
    VAR2/bla: var2-bla
    VAR2/ble: var2-ble

can be thought of as:

environment:
  bla:
    VAR1: var1-bla
    VAR2: var2-bla
  ble:
    VAR1: var1-be
    VAR2: var2-ble


restore: |
if [[ -n "$PROJECT_DIR" ]]; then
cd "$PROJECT_DIR"
fi

Comment on lines +14 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fi
fi
snapcraft clean

rm -f ./*.snap
rm -rf ./snap

execute: |
# unset SNAPCRAFT_BUILD_ENVIRONMENT=host
unset SNAPCRAFT_BUILD_ENVIRONMENT

# initialize a new snapcraft project
snapcraft init
args=("init")

if [[ -n "$PROFILE" ]]; then
args+=("--profile" "$PROFILE")
fi

if [[ -n "$PROJECT_DIR" ]]; then
args+=("$PROJECT_DIR")
fi

snapcraft "${args[@]}"

if [[ -n "$PROJECT_DIR" ]]; then
cd "$PROJECT_DIR"
fi

# the base should be core24
grep "^base: core24" snap/snapcraft.yaml
Expand Down
Loading
Loading