#1033 add default architecture support for format_mmd
Merged 8 months ago by mprahl. Opened 8 months ago by vmaljulin.
vmaljulin/fm-orchestrator FACTORY-1935  into  master

file modified
+1 -1

@@ -28,7 +28,7 @@ 

      MESSAGING_TOPIC_PREFIX = ['org.fedoraproject.prod']

      KOJI_CONFIG = '/etc/module-build-service/koji.conf'

      KOJI_PROFILE = 'koji'

-     KOJI_ARCHES = ['i686', 'armv7hl', 'x86_64']

+     ARCHES = ['i686', 'armv7hl', 'x86_64']

      KOJI_REPOSITORY_URL = 'https://kojipkgs.fedoraproject.org/repos'

      KOJI_TAG_PREFIXES = ['module']

      KOJI_ENABLE_CONTENT_GENERATOR = True

@@ -51,7 +51,7 @@ 

  

  from module_build_service.builder.base import GenericBuilder

  from module_build_service.builder.KojiContentGenerator import KojiContentGenerator

- from module_build_service.utils import get_reusable_components, get_reusable_module

+ from module_build_service.utils import get_reusable_components, get_reusable_module, get_build_arches

  

  logging.basicConfig(level=logging.DEBUG)

  

@@ -170,19 +170,10 @@ 

          log.debug("Using koji_config: %s" % config.koji_config)

  

          self.koji_session = self.get_session(config, owner)

-         self.arches = config.koji_arches

- 

-         # Handle BASE_MODULE_KOJI_ARCHES. Find out the base modules in buildrequires

-         # section of XMD and set the Koji tag arches according to it.

-         if "mbs" in self.mmd.get_xmd().keys():

-             for req_name, req_data in self.mmd.get_xmd()["mbs"]["buildrequires"].items():

-                 ns = ":".join([req_name, req_data["stream"]])

-                 if ns in config.base_module_koji_arches:

-                     self.arches = config.base_module_koji_arches[ns]

-                     break

+         self.arches = get_build_arches(self.mmd, self.config)

  

          if not self.arches:

-             raise ValueError("No koji_arches specified in the config.")

+             raise ValueError("No arches specified in the config.")

  

          # These eventually get populated by calling _connect and __prep is set to True

          self.module_tag = None  # string

@@ -388,12 +388,12 @@ 

  

          :param components: List of comopnent names to compute the weight for.

          :param arches: List of arches to build for or None. If the value is None,

-             conf.koji_arches will be used instead.

+             conf.arches will be used instead.

          :rtype: dict

          :return: {component_name: weight_as_float, ...}

          """

          if not arches:

-             arches = conf.koji_arches

+             arches = conf.arches

  

          weights = {}

  

@@ -175,7 +175,7 @@ 

              'type': str,

              'default': None,

              'desc': 'Koji config profile.'},

-         'koji_arches': {

+         'arches': {

This is a breaking change. I think we should have this be backwards compatible or else we'll have to release v3.0.0 and change our Ansible playbooks.

              'type': list,

              'default': [],

              'desc': 'Koji architectures.'},

@@ -415,7 +415,7 @@ 

              'default': ['platform'],

              'desc': ("List of base module names which define the product version "

                       "(by their stream) of modules depending on them.")},

-         'base_module_koji_arches': {

+         'base_module_arches': {

              'type': dict,

              'default': {},

              'desc': 'Per base-module name:stream Koji arches list.'},

@@ -642,15 +642,15 @@ 

                               .format(strategy, ', '.join(SUPPORTED_STRATEGIES)))

          self._rebuild_strategy = strategy

  

-     def _setifok_base_module_koji_arches(self, data):

+     def _setifok_base_module_arches(self, data):

          if not isinstance(data, dict):

-             raise ValueError("BASE_MODULE_KOJI_ARCHES must be a dict")

+             raise ValueError("BASE_MODULE_ARCHES must be a dict")

          for ns, arches in data.items():

              if len(ns.split(":")) != 2:

-                 raise ValueError("BASE_MODULE_KOJI_ARCHES keys must be in 'name:stream' format")

+                 raise ValueError("BASE_MODULE_ARCHES keys must be in 'name:stream' format")

              if not isinstance(arches, list):

-                 raise ValueError("BASE_MODULE_KOJI_ARCHES values must be lists")

-         self._base_module_koji_arches = data

+                 raise ValueError("BASE_MODULE_ARCHES values must be lists")

+         self._base_module_arches = data

  

      def _setifok_rebuild_strategies_allowed(self, strategies):

          if not isinstance(strategies, list):

@@ -355,3 +355,24 @@ 

                          whitelist_url=False, mandatory_checks=False)

  

      return mmd

+ 

+ 

+ def get_build_arches(mmd, config):

+     """

+     Returns the list of architectures for which the module `mmd` should be built.

+ 

+     :param mmd: Module MetaData

+     :param config: config (module_build_service.config.Config instance)

+     :return list of architectures

+     """

+     arches = config.arches

+ 

+     # Handle BASE_MODULE_ARCHES. Find out the base modules in buildrequires

+     # section of XMD and set the Koji tag arches according to it.

+     if "mbs" in mmd.get_xmd().keys():

+         for req_name, req_data in mmd.get_xmd()["mbs"]["buildrequires"].items():

+             ns = ":".join([req_name, req_data["stream"]])

+             if ns in config.base_module_arches:

+                 arches = config.base_module_arches[ns]

+                 break

+     return arches

@@ -173,6 +173,10 @@ 

                  pkg.set_cache(conf.rpms_default_cache + pkgname)

              if not pkg.get_ref():

                  pkg.set_ref('master')

+             if pkg.get_arches().size() == 0:

+                 arches = Modulemd.SimpleSet()

+                 arches.set(conf.arches)

+                 pkg.set_arches(arches)

  

          # Add missing data in included modules components

          for modname, mod in mmd.get_module_components().items():

@@ -412,7 +412,7 @@ 

          weights = KojiModuleBuilder.get_build_weights(["httpd", "apr"])

          assert weights == {"httpd": 1.5, "apr": 1.5}

  

-     @patch.object(conf, 'base_module_koji_arches',

+     @patch.object(conf, 'base_module_arches',

                    new={"platform:xx": ["x86_64", "i686"]})

      @pytest.mark.parametrize('blocklist', [False, True])

      @pytest.mark.parametrize('custom_whitelist', [False, True])

@@ -542,6 +542,45 @@ 

              for c in module_build.component_builds:

                  assert c.weight == 1.5

  

+     @patch('module_build_service.scm.SCM')

+     def test_format_mmd_arches(self, mocked_scm):

+         with app.app_context():

+             clean_database()

+             mocked_scm.return_value.commit = \

+                 '620ec77321b2ea7b0d67d82992dda3e1d67055b4'

+             mocked_scm.return_value.get_latest.side_effect = [

+                 '4ceea43add2366d8b8c5a622a2fb563b625b9abf',

+                 'fbed359411a1baa08d4a88e0d12d426fbf8f602c',

+                 'dbed259411a1baa08d4a88e0d12d426fbf8f6037',

+                 '4ceea43add2366d8b8c5a622a2fb563b625b9abf',

+                 'fbed359411a1baa08d4a88e0d12d426fbf8f602c',

+                 'dbed259411a1baa08d4a88e0d12d426fbf8f6037']

+ 

+             testmodule_mmd_path = path.join(

+                 BASE_DIR, '..', 'staged_data', 'testmodule.yaml')

+             test_archs = ['powerpc', 'i486']

+ 

+             mmd1 = Modulemd.Module().new_from_file(testmodule_mmd_path)

+             mmd1.upgrade()

+ 

+             module_build_service.utils.format_mmd(mmd1, None)

+ 

+             for pkg in mmd1.get_rpm_components().values():

+                 assert set(pkg.get_arches().get()) == set(conf.arches)

+ 

+             mmd2 = Modulemd.Module().new_from_file(testmodule_mmd_path)

+             mmd2.upgrade()

+ 

+             new_arches = Modulemd.SimpleSet()

+             new_arches.set(test_archs)

+             for pkg in mmd2.get_rpm_components().values():

+                 pkg.set_arches(new_arches)

+ 

+             module_build_service.utils.format_mmd(mmd2, None)

+ 

+             for pkg in mmd2.get_rpm_components().values():

+                 assert set(pkg.get_arches().get()) == set(test_archs)

+ 

      def test_generate_koji_tag_in_nsvc_format(self):

          name, stream, version, context = ('testmodule', 'master', '20170816080815', '37c6c57')

  

Add a default architecture to each package in module metadata according to the config.

Does pkg.get_arches().size() == 0 work?

Does pkg.get_arches().size() == 0 work?

yes, size returns 0 for the empty set

Does pkg.get_arches().size() == 0 work?

yes, size returns 0 for the empty set

If it's the same as not pkg.get_arches().get() I would rather use the size check to avoid creating a temporary list (not sure how the set work internally).

This is unrelated to the test you added this to (test_record_component_builds_set_weight). Could you please make this a separate test?

Is the goal of this test to make sure the arches aren't overwritten?

@mprahl, I agree, but I would maybe rather rename the test instead. Doing separate tests for each attribute record_component_builds sets is useful, but it would possible result in lot of tests to test the default behaviour of that method.

Could you perhaps consolidate the changes you made in the previous test to this one, where you manually set some of the arches and leave some of the arches as None, and then you can check that the end result is what you want.

1 new commit added

  • fixup! add architectures support for format_mmd
8 months ago

2 new commits added

  • fixup! add architectures support for format_mmd
  • add architectures support for format_mmd
8 months ago

rebased onto fb0c7cd178fd6a564608d3a84061bd7116fd7e8d

8 months ago

1 new commit added

  • fixup! add architectures support for format_mmd
8 months ago

rebased onto e6e0834276f1605ef5683ac5438e977aceaf9e9b

8 months ago

I guess it's ready to merge now...

rebased onto feb738afeb8320ee7f61d1154b5264f34873a79d

8 months ago

rebased onto feb738afeb8320ee7f61d1154b5264f34873a79d

8 months ago

The config you are using is specific to the Koji builder, but this code is supposed to be builder agnostic.

@jkaluza, do you think we should just rename conf.koji_arches to conf.arches?

@mprahl, when thinking about this, we should probably move following code from KojiModuleBuilder.py to ./utils/general.py and find out some good name like get_build_arches(mmd):

        self.arches = config.koji_arches

        # Handle BASE_MODULE_KOJI_ARCHES. Find out the base modules in buildrequires
        # section of XMD and set the Koji tag arches according to it.
        if "mbs" in self.mmd.get_xmd().keys():
            for req_name, req_data in self.mmd.get_xmd()["mbs"]["buildrequires"].items():
                ns = ":".join([req_name, req_data["stream"]])
                if ns in config.base_module_koji_arches:
                    self.arches = config.base_module_koji_arches[ns]
                    break

And then use this get_build_arches(mmd) here and also in KojiModuleBuilder instead of the moved code. As follow-up, we can rename base_module_koji_arches to base_module_arches and koji_arches to just arches.

@mprahl, when thinking about this, we should probably move following code from KojiModuleBuilder.py to ./utils/general.py and find out some good name like get_build_arches(mmd):
self.arches = config.koji_arches

    # Handle BASE_MODULE_KOJI_ARCHES. Find out the base modules in buildrequires
    # section of XMD and set the Koji tag arches according to it.
    if "mbs" in self.mmd.get_xmd().keys():
        for req_name, req_data in self.mmd.get_xmd()["mbs"]["buildrequires"].items():
            ns = ":".join([req_name, req_data["stream"]])
            if ns in config.base_module_koji_arches:
                self.arches = config.base_module_koji_arches[ns]
                break

And then use this get_build_arches(mmd) here and also in KojiModuleBuilder instead of the moved code. As follow-up, we can rename base_module_koji_arches to base_module_arches and koji_arches to just arches.

@jkaluza, that makes sense. My concern with that loop is that the algorithm is inconsistent if there is more than one entry in config.base_module_koji_arches[ns] since dict.items() can have a different order each time it is executed. Perhaps looping through conf.base_module_names first and then checking to see if that entry is in self.mmd.get_xmd()["mbs"]["buildrequires"] and then if that name:stream is in config.base_module_koji_arches.

1 new commit added

  • FACTORY-1935: renaming koji_arches to arches
8 months ago

1 new commit added

  • FACTORY-1935: renaming koji_arches to arches
8 months ago

This is a breaking change. I think we should have this be backwards compatible or else we'll have to release v3.0.0 and change our Ansible playbooks.

Don't you want to break after this?

2 new commits added

  • FACTORY-1935: renaming koji_arches to arches
  • add architectures support for format_mmd
8 months ago

2 new commits added

  • FACTORY-1935: renaming koji_arches to arches
  • add architectures support for format_mmd
8 months ago

2 new commits added

  • renaming koji_arches to arches
  • add architectures support for format_mmd
8 months ago

2 new commits added

  • renaming koji_arches to arches
  • add architectures support for format_mmd
8 months ago

Can you write comment here like:

"""
Returns the list of architectures for which the module `mmd` should be built.
"""

@mprahl, given we are really the only one running MBS KojiModuleBuilder, I would just release this without bumping to 3.0.0. But I don't mind doing that too. Making this backward-compatible with introducing "deprecated" warning seems to be too much work for such change deployed only in infra by us.

@mprahl, given we are really the only one running MBS KojiModuleBuilder, I would just release this without bumping to 3.0.0. But I don't mind doing that too. Making this backward-compatible with introducing "deprecated" warning seems to be too much work for such change deployed only in infra by us.

Fair enough. We need to be conscious of this change though when we do make a new release.

:thumbsup: after you add the docstring requested by @jkaluza

2 new commits added

  • renaming koji_arches to arches
  • add architectures support for format_mmd
8 months ago

rebased onto 924a095

8 months ago

So, who will merge this?

Pull-Request has been merged by mprahl

8 months ago