In the new modulemd case, you are catching 'arches.get()' - self.arches is a list, and doesn't have a get method. You want to keep this kind of try/except really tight.

try:
    arches = buildopt.get_arches()
except AttributeError:
   arches  = None

In addition to the above, a test case addition would likely be a good idea as well. I just checked, and using mock.patch("gi.repository.Modulemd.Buildopts.get_arches", side_effect=AttributeError) should work if you want to test both branches with a new libmodulemd.

@breilly could you please add a unit test for this?

Thanks, updated.

Could you please move this code to the get_build_arches function? That way the architectures associated with the module build in the database are influenced by the modulemd file.

Edit: The correct function is get_build_arches in submit.py.

How about buildopts_arches = [] here instead? Then you can remove buildopts_arches = None and if buildopts_arches:.

Thanks @mprahl, updated.

This section of the code is for when arches are configured for a specific base module in the MBS configuration. An example of when this is used is when a new version of Fedora drops one or more architectures. This happened for Fedora 31 when i686 was dropped.

My suggestion is to change this function so that when a set of architectures are found, you then check to see if the modulemd of the module being built has architectures set. If it does, you need to verify that the architectures on the modulemd are a subset of the architectures that were just found. If they are, return the architectures on the modulemd file. If they are not a subset, then you need to raise a ValidationError explaining the issue which will cause the module build to fail. If the modulemd doesn't have the architectures set, then just return the found architectures like the existing behavior.

Could you move this to under the if branch since the variable is only used if buildopts is truthy? You could also just replace pass with buildopts_arches = [].

Could you use set theory here instead of a for loop? For instance:

unsupported_arches = set(buildopts_arches) - set(arches)
if unsupported_arches:
    # Raise some error that lets the user know which arches aren't supported

Thanks, updated

Optional: You could do the following to avoid the long if branch:

if not buildopts:
   return arches

It'd be nice to have a docstring, especially explaining arches.

Instead of passing in nsvc, I believe you can just call mmd.get_nsvc().

Why is checking the length relevant here?

I would think that if buildopts_arches is set and compatible, then it should be returned

Edit: It looks like you are trying to see if the arches are different before logging. In that case, just do:

if set(arches) != set(buildopts_arches):

Edit 2: On second thought, I don't think should be checking this at all. If buildopts_arches is set and compatible, it should be used, even if its the same. It'd be confusing otherwise since the log messages would imply that buildopts_arches was not consulted when it's the same as what the arches would have been.

You could probably just return arches here

I wanted to check if the restricted arches were the same as the ones gotten from the original means, for the sake of a correct log message (since it would essentially be the same thing as setting based on the base module arches for instance). Length was just a quick way to do that.

Checking the length is not a good way to determine if the arches have changed. Just do:

if set(arches) == set(new_arches):

Alternatively, you can check to see if arches was returned as is by _check_buildopts_arches:

if arches is new_arches:

Same as above

Same as above

Same as above

This can be removed

How about something like this?
test_get_build_arches_limited => test_get_build_arches_set_in_mmd

Optional: There was an effort to keep the imports in alphabetical order

We'd still want to avoid a situation in which both strings from _check_buildopts_arches and get_build_arches are printed, right? Which would happen in the case of the limited arch set being the same as the original set, if the check in _check_buildopts_arches is removed. Is there a better solution here?

Thanks, updated.

A couple ways come to mind to avoid the above situation: 1) pass the log message into the function or 2) return a boolean value in a tuple. Both of those seem very inelegant, so I haven't implemented either. Open to suggestions on this.

How about?

if arches is new_arches:

This would check to see if arches and new_arches point to the same memory address. If so, then you know _check_buildopts_arches didn't return the arches from the modulemd.

:return => :return:

the platform module arches => the input module arches

Optional: Since this logic is repeated, you could create a small function inside this function:

def _conditional_log(msg, arches, new_arches):
    if arches is new_arches:
        log.info(msg)

Ah, right, checking the memory address was exactly what was needed. Thanks!
Updated.

It'd be nice to include a comment explaining this :smile:

Optional: Instead of str(unsupported_arches), I think you can just use unsupported_arches directly but instead of %s, you use %r.

mmd.get_nsvc => mmd.get_nsvc()

How about something like?

raise ValueError("The following buildopts arches are not supported with these buildrequires: %r" % unsupported_arches)

The main thing is that we should avoid saying platform since it's customary to use that name for the a base module, but it's not hard coded logic in MBS. Additionally, things other than the buildrequired platform module can influence the build. This is true for privileged modules that are buildrequired (used for layered product module builds).

This should also raise a ValidationError instead of ValueError. This is because in the modules.py::init function, it will only use the message from certain exception types for the module's state_reason when it's transitioned to the failed state.

@breilly sorry I missed a few other things in the last review. Once these comments are addressed, I think this will be good to merge.

Thanks, updated

Could you just return buildopts_arches here?

In general, I find it to be a bad practice to overwrite function parameters since it makes code harder to read. This forces you to have to read the whole function to see where the variable could change, and it might not be expected by the reader if they are reading a small section within the function.

@breilly just one minor comment.

Thanks @mprahl, updated.

Optional: This is more readable:

mbs_options = xmd.get("mbs", {})

Optional: It'd be nice if this was a single commit. There's no use having a separate commit for the unit test. It just makes the git history more cluttered and the change harder to revert.

:thumbsup:

@breilly this should be:

mbs_options = xmd.get("mbs", {})

:thumbsup:

This fixes https://pagure.io/fm-orchestrator/issue/1557