-
-
Notifications
You must be signed in to change notification settings - Fork 389
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
smcv
wants to merge
16
commits into
flatpak:main
Choose a base branch
from
smcv:split-utils-repo
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]>
Signed-off-by: Simon McVittie <[email protected]>
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]>
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]>
Signed-off-by: Simon McVittie <[email protected]>
GeorgesStavracas
approved these changes
May 29, 2024
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.
Looks good to me, only one small comment about translation files, should be easy to address
if (current_ref) | ||
return g_steal_pointer (¤t_ref); | ||
|
||
flatpak_fail_error (error, FLATPAK_ERROR_NOT_INSTALLED, _("%s not installed"), app_id); |
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 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Limitations:
flatpak_deploy_data_get_commit()
, while -dir callsflatpak_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