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

Further reduce circular dependencies #5803

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

smcv
Copy link
Collaborator

@smcv smcv commented May 2, 2024

Continuation of #5409, #5801. Any prefix of this branch should be OK to apply, so it can be broken into smaller pieces if reviewers would prefer that.

The overall goal of this branch is to break up unmanageably large translation units, and make the common/ code more understandable by separating it into layers (utils < exports < context, utils < xml-utils < repo-utils < dir, etc.) instead of having all modules call into all other modules.

My ulterior motive for this refactoring is that my day job involves maintaining Steam's pressure-vessel tool, which reuses some of the lower-level parts of Flatpak (notably -utils, -exports and some of -run) but not the higher-level parts (in particular anything with libostree in it). At the moment we do that with a lot of #if 0 around the parts we don't want, but if I could copy the lower-level translation units in their entirety while leaving out the ones that would add dependencies, that would be a lot easier!

But, even without that factor, I think this PR is an improvement to Flatpak - having it be layered rather than entangled makes it easier for contributors and maintainers to understand the overall system.

Having that layering should also allow us to drop some of the library dependencies from Flatpak's own ancillary tools: for example, there's no real reason why the portal and the system helper should need dependencies on libostree, libsoup/libcurl or libarchive, but at the moment they have to link libflatpak_common_dep which depends on those.

I think the version in this PR achieves reasonably good layering, especially in the lower layers, for example:

  • libglnx < glib-backports < everything else
  • metadata < everything
  • errors < utils < almost everything else
  • utils < appdata
  • utils < bwrap < exports < context < run
  • utils < instance
  • utils < ref-utils < repo-utils < bundle-ref
  • utils < ref-utils < repo-utils < {dir, dir-utils} < installation
  • ... < {dir, dir-utils} < installed-ref
  • ... < {dir, dir-utils} < oci-registry
  • ... < {dir, dir-utils} < related-ref
  • ... < {dir, dir-utils} < remote

Limitations:

  • The higher-level "package management" modules auth, dir and dir-utils are currently still entangled. My knowledge of the "package manager" layer of Flatpak is limited, so I'd prefer it if someone who is better at the conceptual design of Flatpak can disentangle that part.
  • -run and -dir are also still entangled, because -run calls flatpak_deploy_data_get_commit(), while -dir calls flatpak_run_get_minimal_env() among others. This PR is already large, so I'd prefer to disentangle that another time!

Commits

  • utils: Include flatpak-metadata-private.h instead of -run-private.h

    This avoids a circular dependency between -run and -utils.

  • common: Move OCI registry manipulation into FlatpakOciRegistry

    This is a step towards making flatpak-utils conceptually "smaller"
    than all other translation units, with no dependencies beyond GLib and
    libglnx. In particular, consolidating all the OCI registry manipulation
    into one place means we can build other translation units without
    libarchive.

    This would also be a step towards being able to provide a build-time
    option to build a libostree-only version of Flatpak without the OCI
    feature or the direct libarchive dependency, if someone wanted to
    implement that.

  • repo-utils: New header for some implementation details of a repository

    This will reduce circular dependencies involving FlatpakDir.

  • common: Break out the parts of flatpak-utils that deal with FlatpakDir

    This breaks the circular dependency between flatpak-utils and flatpak-dir.
    There is still a circular dependency between flatpak-dir and
    flatpak-dir-utils, but I don't want to make flatpak-dir even larger.

  • common: Explicitly include ostree.h where needed

    A subsequent commit will remove it from flatpak-utils-private.h.

  • utils: Move OstreeRepo configuration accessors to a new translation unit

    This is a step towards removing the libostree dependency from
    flatpak-utils, which should be one of the lowest-level components.

  • ref-utils: Move flatpak_get_arch_for_ref() to here

    The declaration was already in flatpak-ref-utils-private.h.

    Fixes: 5dae1fc "Break out ref helper functions to separate file"

  • dir: Move some more implementation details into repo-utils

    This allows repo-utils to be kept lower-level than dir.

  • utils: Move more repository functionality into repo-utils

  • utils: Move more summary parsing into repo-utils

  • utils: Export flatpak_get_compat_arch()

    This will allow its caller to be moved into repo-utils.

  • utils: Move more repository functionality to repo-utils

    This further reduces circular dependencies: utils no longer has a
    circular dependency with repo-utils or xml-utils.

  • utils: Move remaining direct ostree dependencies to repo-utils

  • utils: Remove unnecessary flatpak-ref-utils-private.h inclusion

    Include flatpak-ref-utils-private.h explicitly in each remaining
    module that needs it (mostly for FlatpakDecomposed).

  • prune: Include flatpak-variant-private.h before its -impl-private.h

    This ensures that declarations are visible.

  • utils: Remove flatpak-variant-private.h, no longer necessary

@smcv
Copy link
Collaborator Author

smcv commented May 3, 2024

@TingPing, if you liked #5801 then this might also be of interest.

smcv added 16 commits May 6, 2024 16:06
This avoids a circular dependency between -run and -utils.

Signed-off-by: Simon McVittie <[email protected]>
This is a step towards making flatpak-utils conceptually "smaller"
than all other translation units, with no dependencies beyond GLib and
libglnx. In particular, consolidating all the OCI registry manipulation
into one place means we can build other translation units without
libarchive.

This would also be a step towards being able to provide a build-time
option to build a libostree-only version of Flatpak without the OCI
feature or the direct libarchive dependency, if someone wanted to
implement that.

Signed-off-by: Simon McVittie <[email protected]>
This will reduce circular dependencies involving FlatpakDir.

Signed-off-by: Simon McVittie <[email protected]>
This breaks the circular dependency between flatpak-utils and flatpak-dir.
There is still a circular dependency between flatpak-dir and
flatpak-dir-utils, but I don't want to make flatpak-dir even larger.

Signed-off-by: Simon McVittie <[email protected]>
A subsequent commit will remove it from flatpak-utils-private.h.

Signed-off-by: Simon McVittie <[email protected]>
This is a step towards removing the libostree dependency from
flatpak-utils, which should be one of the lowest-level components.

Signed-off-by: Simon McVittie <[email protected]>
The declaration was already in flatpak-ref-utils-private.h.

Fixes: 5dae1fc "Break out ref helper functions to separate file"
Signed-off-by: Simon McVittie <[email protected]>
This allows repo-utils to be kept lower-level than dir.

Signed-off-by: Simon McVittie <[email protected]>
This will allow its caller to be moved into repo-utils.

Signed-off-by: Simon McVittie <[email protected]>
This further reduces circular dependencies: utils no longer has a
circular dependency with repo-utils or xml-utils.

Signed-off-by: Simon McVittie <[email protected]>
Include flatpak-ref-utils-private.h explicitly in each remaining
module that needs it (mostly for FlatpakDecomposed).

Signed-off-by: Simon McVittie <[email protected]>
This ensures that declarations are visible.

Signed-off-by: Simon McVittie <[email protected]>
Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

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

Looks good to me, only one small comment about translation files, should be easy to address

if (current_ref)
return g_steal_pointer (&current_ref);

flatpak_fail_error (error, FLATPAK_ERROR_NOT_INSTALLED, _("%s not installed"), app_id);
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to add this file to POTFILES.in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants