why not to do this one... in __init__.py?

I deprecated this function in libmodulemd 1.2.0 because it's confusing. It returns both Module and Defaults objects, and as such should not be a method on the Modules object.

I recommend using the newer, preferred interface Modulemd.objects_from_string()

You need to check for whether you've got a module or a defaults object before this or else this line is going to throw an exception.

Maybe this loop should be a list comprehension instead that checks the type.

Do we actually want this to be explicitly f28 here? Should it instead be using the module stream expansion form?
dependencies.add_buildrequires(modname, [])

Oh, and the other question is whether it makes sense to have 'f28' be the default here, since really that only applies to the platform module?

Is this intended to only provide a starting point? Seems odd that buildorder is always only 0 or 10.

why not to do this one... in __init__.py?

My thought was "this is the generic recipe for requiring libmodulemd" and hiding part of the recipe in some other file would be confusing if someone wanted to borrow code. But I agree that it would make things a little shorter and cleaner looking to put it in a central place, and also repeating does give the mistaken impression that you could the version in only one file and have it work.

Is this intended to only provide a starting point? Seems odd that buildorder is always only 0 or 10.

That's orthogonal to this patch :-) The existing fedmod code builds all the implicit dependencies of what was specified on the rpm2flatpak command first, and then the explicitly specified packages afterwards.

I have code to do more real build-ordering (discussion in issue #48)

Is this intended to only provide a starting point? Seems odd that buildorder is always only 0 or 10.

That's orthogonal to this patch :-) The existing fedmod code builds all the implicit dependencies of what was specified on the rpm2flatpak command first, and then the explicitly specified packages afterwards.
I have code to do more real build-ordering (discussion in issue #48)

Sure, I just spotted that in the stream of work and was curious. Don't treat that as blocking for the review.

Do we actually want this to be explicitly f28 here? Should it instead be using the module stream expansion form? dependencies.add_buildrequires(modname, [])
Oh, and the other question is whether it makes sense to have 'f28' be the default here, since really that only applies to the platform module?

This is obviously a policy question, but that sounds like it corresponds to where I understand the policy is going for general modules. I'll change it that way. Flatpaks are different - they target a particular fedora-versioned Flatpak runtime, but I'll handle that in the flatpak2rpm patch.

This is correct because the content licenses should not be provided by the module creator; they're added by the build process by detecting the licenses in the component RPMs.

One more comment: please update fedmod.spec and README.md to reference the change in dependency from python3-modulemd to libmodulemd and python3-gobject-introspection.

One more comment: please update fedmod.spec and README.md to reference the change in dependency from python3-modulemd to libmodulemd and python3-gobject-introspection.

Fixed those and setup.py

LGTM. Ack

not needed

not needed

not needed

not needed

Thanks for catching the leftovers. Fixed.

This PR breaks some tests:

pytest -vv tests

================================================================================================================= FAILURES ==================================================================================================================
_____________ TestSinglePackageInput.test_generated_modulemd_file _____________

self =

def test_generated_modulemd_file(self):
    input_rpms = ('grep',)
    modmd = _generate_modulemd(input_rpms)
    # Expected description for 'grep'
    assert modmd.props.summary == "Generated module for grep"
    assert modmd.props.description == "Module auto-generated by fedmod"
    # Expected licenses for 'grep'

  assert modmd.props.module_licenses == {'MIT'}

E AssertionError: assert == {'MIT'}
E + where = .module_licenses
E + where = .props

tests/test_module_generator.py:33: AssertionError
_____________ TestMultiplePackageInput.test_generated_modulemd_file _____________

self =

def test_generated_modulemd_file(self):
    input_rpms = ('grep', 'haproxy')
    modmd = _generate_modulemd(input_rpms)
    # Can't generate descriptive metadata when given multiple RPMs
    assert modmd.props.summary == "Generated module for ('grep', 'haproxy')"
    assert modmd.props.description == "Module auto-generated by fedmod"
    # Expected licenses for grep + haproxy

  assert modmd.props.module_licenses == {'MIT'}

E AssertionError: assert == {'MIT'}
E + where = .module_licenses
E + where = .props

tests/test_module_generator.py:66: AssertionError

Sorry for the slow response - was on PTO for a few weeks. I'm not sure where the code you are hitting the failures above comes from

  assert modmd.props.module_licenses == {'MIT'}

In this PR, that code is:

  assert modmd.props.module_licenses.get() == ['MIT']

Maybe there was some sort of merge conflict with local changes that you fixed when you were testing this?

you're right, sorry!