feat!(services): add a project service #646

lengau · 2025-02-13T23:49:00Z

Have you followed the guidelines for contributing?
Have you signed the CLA?
Have you successfully run make lint && make test?
Have you added an entry to the changelog (docs/reference/changelog.rst)?

This creates a ProjectService for handling projects. There are a few tests that this causes issues with because it messes with build plans, so those are being skipped until I implement the BuildPlanService.

This is pretty huge, so I'm providing a review methodology. I'd recommend marking each file as viewed as you complete it, even if you have comments, because there's a "draw the rest of the owl" step at the end.

Start with the Project service loading docs (rendered), which should provide you a good overview of how the project service works.
Review the ProjectService itself. This is the actual core of the new logic.
Review the project service's unit tests
Review its integration tests, including the example input/output files.
Review the changes to the Application, which remove project rendering. Here's a good spot to look for anything I may have overlooked when moving the logic.
Review the changes to the other services. I'll put in some implementation notes that are only really relevant when considering the changes.
Review the new fixtures for this.
Note that we no longer have a ProjectService abstract class
Likewise, the Service factory now no longer checks for a project before creating services
Review the rest of the services
Review any files you haven't already reviewed.

CRAFT-4158

lengau · 2025-02-21T16:45:30Z

craft_application/services/project.py

@cmatsuoka this is the bulk of the actual change to the business logic. It (and its related test) is already reviewable.

Looking good so far.

This makes the ServiceFactory no longer a dataclass. It's only a dataclass for historical reasons around the old way of registering services, but this is no longer necessary.

We don't always have that call at the beginning of the interactions.

…vice

Removes the ability to use secrets. This can be added to applications if necessary using the prototype secrets service.

craft_application/services/service_factory.py

lengau · 2025-02-21T20:51:51Z

docs/reference/services/index.rst

This might be a bit much infrastructure for now, but I'd rather have it than need to make forwards later.

lengau · 2025-02-21T20:52:39Z

craft_application/services/project.py

+        self._project_dir = project_dir
+        self._project_model = None
+
+    def resolve_project_file_path(self) -> pathlib.Path:


Please review these docstrings both as-is and in the context of the autodocs.

sergiusens · 2025-02-24T13:17:23Z

craft_application/services/project.py

+
+
+class ProjectService(base.AppService):
+    """A service for handling access to the project."""


I feel this to be scant but as I tried to write something up, I could not think of anything meaningful

Yeah that's why I didn't include more. Every time I tried to add something it seemed redundant to the documentation page.

Maybe a reference to the documentation page is what I should add?

sergiusens · 2025-02-24T13:20:56Z

craft_application/services/project.py

+        The application must use the provided parameters rather than service state
+        when applying ``build_on``, ``build_for`` or ``platform`` information.


this needs rewording, or expanding

I think it might need removing (and replacing). My first implementation of the ProjectService had those three as instance attributes as well, but this ended up complicating the implementation. I'll rewrite to include some real-world use cases.

sergiusens · 2025-02-24T13:23:56Z

docs/reference/services/project.rst

+---------------
+
+The project service is responsible for loading, validating and rendering the project
+file to a ``Project`` model. While the service can render a project as though it is


can you reference Project even if it is the base model?

I've changed it, though the rendered version won't change because we don't include the class's reference in docs. I've created #662 for that.

craft_application/services/project.py

dariuszd21 · 2025-02-24T18:42:18Z

craft_application/services/project.py

+                arch=build_on,
+                target_arch=str(build_for),


Why one needs str() and another one does not ?

build_for used to be a DebianArchitecture or the string all. The type change is no longer needed.

craft_application/services/project.py

dariuszd21 · 2025-02-24T18:50:23Z

craft_application/services/project.py

+                raise RuntimeError(
+                    "Somehow the host architecture is all Debian architectures. "
+                    "My universe is shattered; I give up."
+                )


Even if this is very unlikely, we should have a dedicated error for that, that Host architecture couldn't be resolved, so we can't build for "all".

I don't think we need a dedicated error, because this is an error in the application. The application would need to provide a version of craft_platforms that only has a single value in DebianArchitecture that matches. I'll rewrite the message to be less prototype-y though.

dariuszd21 · 2025-02-24T18:59:02Z

craft_application/services/service_factory.py

@@ -257,6 +256,8 @@ def get(self, service: Literal["package"]) -> services.PackageService: ...
    @overload
    def get(self, service: Literal["lifecycle"]) -> services.LifecycleService: ...
    @overload
+    def get(self, service: Literal["project"]) -> ProjectService: ...


Why not services.ProjectService ?

Eventually I want to remove the individual services from the services module directly so we don't even have to load built-in services if we don't use them. ProjectService only gets imported here in type-checking mode or when actually requesting it.

tests/integration/services/project_files/testcraft-full.yaml

dariuszd21 · 2025-02-24T19:49:11Z

tests/integration/test_application.py

+    # Workaround until we implement CRAFT-4159
+    app._bootstrap_services()  # We need to access the project service.
+    app.services.get("project").render_once()
+    if date.today() < date(2025, 3, 1):


Is it a good idea to put here 2 weeks deadline 😄 ?

We'll have to extend it if I don't finish the build plan service by the end of the pulse, but I'd rather extend it than have it accidentally go into main.

tests/unit/services/test_init.py

dariuszd21 · 2025-02-24T19:52:33Z

tests/unit/services/test_lifecycle.py

@@ -591,6 +592,7 @@ def test_lifecycle_package_repositories(
        platform=None,
        build_plan=fake_build_plan,
    )
+    service.setup()


Why we need to setup it now?

Because setup gets the project.

dariuszd21 · 2025-02-24T19:55:16Z

tests/unit/services/test_service_factory.py

@@ -251,8 +251,7 @@ def test_get_unregistered_service(factory):


 def test_get_project_service_error(factory):
-    factory.project = None
-    with pytest.raises(ValueError, match="LifecycleService requires a project"):
+    with pytest.raises(RuntimeError, match="Project not rendered yet."):


Is it a good error ? It is suggesting that it will render at some point

I'm not sure what better wording would be appropriate. Maybe "attempted to get the project before it was rendered"?

dariuszd21

I like the idea and how it looks like.
The comments I left are mostly to clarify certain aspects and to see if I understand it correctly.
From what I can see, it shouldn't affect how the commands that conditionally require project works and for the others change is rather transparent (even if non backward-compatible)

tigarmo

This is great :) the review script is specially appreciated! I think this is overall "there" but I had some questions before approving the PR
thanks!

tigarmo · 2025-02-25T19:26:41Z

craft_application/application.py

@@ -300,6 +301,17 @@ def cache_dir(self) -> pathlib.Path:
                f"Unable to create/access cache directory: {err.strerror}"
            ) from err

+    def _bootstrap_services(self) -> None:


I'd like to suggest _configure_early_services(), so that the difference between it and _configure_services is more evident

tigarmo · 2025-02-25T19:28:51Z

craft_application/application.py

@@ -332,6 +344,9 @@ def _resolve_project_path(self, project_dir: pathlib.Path | None) -> pathlib.Pat
        directory. Applications may wish to override this if the project file could be
         in multiple places within the project directory.
        """
+        warnings.warn(
+            DeprecationWarning("NEEDS REMOVING BEFORE MERGE TO MAIN"), stacklevel=1


is this being removed on a later task?

Thanks for catching this! At first I thought it would need to stay until a future task, but it looks like I have indeed removed all usages.

tigarmo · 2025-02-25T19:31:00Z

craft_application/application.py

+            project_service = self.services.get("project")
+            # This branch always runs, except during testing.
+            if not project_service.is_rendered:
+                project_service.render_once()


what about the platform and build_for parameters? How do they end up in the service in this specific code path?

Good catch, thanks!

tigarmo · 2025-02-25T19:41:41Z

craft_application/services/project.py

+        )
+
+    @final
+    def get_platforms(self) -> dict[str, craft_platforms.PlatformDict]:


how come this method is public? Who, do you figure, will need to use this? Other than tests I mean

The BuildPlanService uses it to generate the build plan. This is also the primary reason for having _app_render_legacy_platforms() - the BuildPlanService expects to get a platforms entry that craft-platforms can use to generate the exhaustive build plan.

docs/reference/services/project.rst

craft_application/services/project.py

tigarmo · 2025-02-26T11:39:31Z

craft_application/application.py

@@ -337,21 +337,6 @@ def _configure_services(self, provider_name: str | None) -> None:
            session_policy=self._fetch_service_policy,
        )

-    def _resolve_project_path(self, project_dir: pathlib.Path | None) -> pathlib.Path:


just to double-check: is snapcraft expected to just drop the override on their version? Or are there more changes?

Snapcraft will need to move that override to the ProjectService's `resolve_project_file_path()'. Should work roughly the same though (just has to use the class attribute for project dir rather than a method arg)

My concern is that Snapcraft calls that one extremely early, before even _configure_early_services() has been called

I don't think this is worth sweating too much just now; since this is going to a feature branch we'll have time to assess the changes in Snapcraft before setting anything in stone.

tigarmo

🚀

tigarmo · 2025-02-26T11:41:49Z

Regarding the changelog: Since there are more changes incoming, do you want to draft the changes for the project service now, or do a unified changelog entry once the feature-branch is ready to be merged to main?

lengau · 2025-02-26T15:38:16Z

@tigarmo to your point about the changelog: I'll be requesting a full documentation review before merge to main, so I'm not requesting it right now on the feature branch. However, I have added the relevant changes so I don't forget them. Thanks!

lengau force-pushed the work/project-service branch 6 times, most recently from 3736290 to 5c62503 Compare February 20, 2025 21:19

lengau commented Feb 21, 2025

View reviewed changes

lengau added 22 commits February 21, 2025 13:04

feat!(partial): project service

04bc7b4

refactor(tests/fixtures): create a single default fake project

231ba21

feat(revert): fix service factory removal of attributes

7b01dfa

refactor(ServiceFactory): de-dataclass the ServiceFactory

1cada53

This makes the ServiceFactory no longer a dataclass. It's only a dataclass for historical reasons around the old way of registering services, but this is no longer necessary.

fix(services): make the FetchService use the ProjectService

89563c9

fix(tests): fix InitService tests for fixture changes

4f29d58

fix(lifecycle): make LifecycleService no longer a ProjectService

9e0c751

fix(services/package): make PackageService use ProjectService

a1857c7

fix(services): make ProviderService use ProjectService

6644cf2

fix(tests): fix request service test

a7ae8c5

We don't always have that call at the beginning of the interactions.

fix(services): make RemoteBuildService use ProjectService

cd52083

fix(tests): fix the service factory tests

522716a

test(ProjectService): add expand_environment tests for the ProjectSer…

b2a37b7

…vice

test: add more unit tests for ProjectService

7cc0653

feat!(secrets): remove secrets

cc23eb7

Removes the ability to use secrets. This can be added to applications if necessary using the prototype secrets service.

chore!(application): remove yaml transforms and environment from app

fb7dd89

test: add integration tests for rendering a project

79b8ee2

fix(application): fix some application tests

8008304

test: remove unneeded tests

adb77bc

test: move adopt-info test to project service

87cb9d0

test: ignore unnecessary test

7eff14d

fix(test): fix application tests

9502d62

lengau force-pushed the work/project-service branch from 40d2762 to 9502d62 Compare February 21, 2025 18:05

lengau changed the title ~~feat!(partial): project service~~ feat!(services): add a project service Feb 21, 2025

lengau requested review from sergiusens and dariuszd21 February 21, 2025 20:11

lengau added 2 commits February 21, 2025 15:14

fix(lint): fix linter issue

e513598

fix(tests): ensure tests don't have side effects

39cd8a9

lengau commented Feb 21, 2025

View reviewed changes

craft_application/services/service_factory.py Outdated Show resolved Hide resolved

lengau commented Feb 21, 2025

View reviewed changes

lengau marked this pull request as ready for review February 21, 2025 20:53

sergiusens reviewed Feb 24, 2025

View reviewed changes

dariuszd21 reviewed Feb 24, 2025

View reviewed changes

dariuszd21 approved these changes Feb 24, 2025

View reviewed changes

tigarmo self-requested a review February 25, 2025 15:10

fix: pr suggestions

ccdd3fd

lengau requested a review from sergiusens February 25, 2025 18:30

tigarmo reviewed Feb 25, 2025

View reviewed changes

fix: pr feedback

e38da2d

lengau requested a review from tigarmo February 26, 2025 01:13

upils mentioned this pull request Feb 26, 2025

feat: support grammar in Volumes section canonical/imagecraft#96

Open

3 tasks

tigarmo reviewed Feb 26, 2025

View reviewed changes

tigarmo approved these changes Feb 26, 2025

View reviewed changes

lengau added 3 commits February 26, 2025 10:36

fix: add changelog entries

4fc04fc

fix: tests that used the old name of configure_early_services

5026b63

Merge branch 'feature/hybrid-commands' into work/project-service

7aca291

lengau merged commit 83ca560 into feature/hybrid-commands Feb 27, 2025
15 checks passed

lengau deleted the work/project-service branch February 27, 2025 15:13



		class ProjectService(base.AppService):
		"""A service for handling access to the project."""

		The application must use the provided parameters rather than service state
		when applying ``build_on``, ``build_for`` or ``platform`` information.

feat!(services): add a project service #646

feat!(services): add a project service #646

Conversation

lengau commented Feb 13, 2025 • edited Loading

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

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

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

dariuszd21 left a comment

Choose a reason for hiding this comment

tigarmo 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

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

Choose a reason for hiding this comment

tigarmo left a comment

Choose a reason for hiding this comment

tigarmo commented Feb 26, 2025

lengau commented Feb 26, 2025

lengau commented Feb 13, 2025 •

edited

Loading