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
[modularity] use libmodulemd #1032
[modularity] use libmodulemd #1032
Conversation
@@ -33,6 +32,10 @@ | |||
from dnf.module.repo_module_dict import RepoModuleDict | |||
from dnf.module.repo_module_version import RepoModuleVersion | |||
|
|||
import gi | |||
gi.require_version('Modulemd', '1.0') | |||
from gi.repository import Modulemd |
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.
[Flake8]
[E402] module level import not at top of file
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.
Ignore
|
||
from dnf.module.exceptions import LoadCacheException, MissingYamlException | ||
|
||
gi.require_version('Modulemd', '1.0') | ||
from gi.repository import Modulemd |
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.
[Flake8]
[E402] module level import not at top of file
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.
Ignore
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.
spec dependencies should be (libmodulemd and pythonX-gobject-base)
generally looks good, but I don't see where get_dependencies() is used, is it just broken or dnf doesn't check moduel dependencies? |
@ignatenkobrain it uses |
Yeah, this is problematic.
you should be using get_dependencies(), check that there is only one element (otherwise it's not really valid "built" module), and on first element call get_requires(). |
Sorry for confusing you, but it turned out to be much more complicated: dependencies:
- requires:
gtk: [3]
platform: [f28]
- requires:
gtk: [4]
platform: [-f28] is valid. On solver level it should be represented as |
I have actually implemented code to handle this: https://pagure.io/fm-orchestrator/c/0224f3995022c0f625f025507b4ad15ab11af97e See |
dnf/module/repo_module_dict.py
Outdated
continue | ||
visited.add(requires_ns) | ||
version_dependencies.update(self.get_module_dependency_latest(requires_name, | ||
requires_stream, |
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.
[Flake8]
[E501] line too long (102 > 100 characters)
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.
still doesn't support multiple dependency entires and doesn't support "exclusion" of streams.
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 in general, changes requested:
- consider removing rich deps from spec
- handle excludes properly or explain why it's skipped
dnf.spec
Outdated
BuildRequires: python2-modulemd | ||
Requires: python2-modulemd | ||
BuildRequires: (libmodulemd and python2-gobject-base) | ||
Requires: (libmodulemd and python2-gobject-base) |
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.
Can you avoid using rich deps? We'll have to compile this under RHEL 7.
for requires_name, requires_streams in \ | ||
repo_module_version.requires(): | ||
for requires_stream in requires_streams: | ||
if requires_stream[0] == '-': |
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.
what does this mean? is it an exclude? is it handled somewhere?
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.
yes, that's an exclude in modulemd
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.
dnf cannot handle module excludes yet, that's why it is skipped. As result, rpms of skipped module:stream won't get included
for requires_name, requires_streams in \ | ||
repo_module_version.requires(): | ||
for requires_stream in requires_streams: | ||
if requires_stream[0] == '-': |
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.
an exclude again?
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.
same as above
No description provided.