PR#179: [cli][frontend][python][rpmbuild] unify mock config generator - copr/copr

copr / copr

#179 [cli][frontend][python][rpmbuild] unify mock config generator

Closed 6 years ago by clime. Opened 6 years ago by praiskup.

Unknown source unify-mock-config-generator into master

[cli][frontend][python][rpmbuild] unify mock config generator

Pavel Raiskup • 6 years ago

9ae8829

cli/copr_cli/build_config.py

file removed

-48

		`@@ -1,48 +0,0 @@`
		`- # coding: utf-8`
		`-`
		`- from jinja2 import Environment`
		`-`
		`- template_string = """\`
		`- # This is development/testing only mock profile, not exactly the same as`
		`- # is used on copr builders; but it is basically similar. If you need an`
		`- # exact mock configuration (because you e.g. try to reproduce failed`
		`- # build), such configuration is put alongside the built RPMs.`
		`-`
		`- include('/etc/mock/{{chroot}}.cfg')`
		`-`
		`- config_opts['root'] = '{{project_id}}_{{chroot}}'`
		`- config_opts['chroot_additional_packages'] = '`
		`- {%- for pkg in additional_packages -%}`
		`- {%- if loop.last -%}`
		`- {{ pkg }}`
		`- {%- else -%}`
		`- {{ pkg }} {% endif -%}`
		`- {%- endfor -%}'`
		`-`
		`- {% if repos %}`
		`- config_opts['yum.conf'] += \"\"\"`
		`- {% for repo in repos %}`
		`- [{{ repo.id }}]`
		`- name="{{ repo.name }}"`
		`- baseurl={{ repo.url }}`
		`- gpgcheck=0`
		`- enabled=1`
		`- skip_if_unavailable=1`
		`- metadata_expire=0`
		`- cost=1`
		`- best=1`
		`- {% endfor %}`
		`- \"\"\"`
		`- {% endif %}`
		`- """`
		`-`
		`- class MockProfile(object):`
		`- def __init__(self, data):`
		`- self.data = data`
		`-`
		`- def __str__(self):`
		`- template = Environment().from_string(template_string)`
		`- return template.render(self.data)`
		`-`
		`-`
		`-`

cli/copr_cli/main.py

file modified

+2 -2

		`@@ -31,9 +31,9 @@`
		`from copr import CoprClient`
		`from copr.client.responses import CoprResponse`
		`import copr.exceptions as copr_exceptions`
		`+ from copr.util import BuildConfig`

		`from .util import ProgressBar`
		`- from .build_config import MockProfile`

		`import pkg_resources`

		`@@ -380,7 +380,7 @@`
		`sys.stderr.write(result.error + "\n")`
		`sys.stderr.write("Un-expected data returned, please report this issue\n")`

		`- print(MockProfile(result.build_config))`
		`+ print(BuildConfig(result.build_config).to_mock_config())`


		`@check_username_presence`

frontend/coprs_frontend/coprs/helpers.py

file modified

+2 -1

		`@@ -611,5 +611,6 @@`
		`'additional_packages': packages.split(),`
		`'repos': repos,`
		`'chroot': chroot_id,`
		`- 'use_bootstrap_container': copr.use_bootstrap_container`
		`+ 'use_bootstrap_container': copr.use_bootstrap_container,`
		`+ 'enable_net': copr.build_enable_net,`
		`}`

frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py

file modified

+1 -9

		`@@ -138,23 +138,15 @@`
		`"project_owner": task.build.copr.owner_name,`
		`"project_name": task.build.copr.name,`
		`"submitter": task.build.user.name if task.build.user else None, # there is no user for webhook builds`
		`- "chroot": task.mock_chroot.name,`

		`- "repos": task.build.repos,`
		`"memory_reqs": task.build.memory_reqs,`
		`"timeout": task.build.timeout,`
		`"enable_net": task.build.enable_net,`
		`- "git_repo": task.build.package.dist_git_repo,`
		`- "git_hash": task.git_hash,`
		`"source_type": helpers.BuildSourceEnum("scm"),`
		`"source_json": json.dumps(`
		`{'clone_url': task.build.package.dist_git_clone_url, 'committish': task.git_hash}),`

		`- "package_name": task.build.package.name,`
		`- "package_version": task.build.pkg_version,`
		`- "repos": build_config.get("repos"),`
		`- "buildroot_pkgs": build_config.get("additional_packages"),`
		`- "use_bootstrap_container": build_config.get("use_bootstrap_container")`
		`+ "build_config": build_config,`
		`}`

		`except Exception as err:`

python/copr/util.py

file modified

+108

		`@@ -1,5 +1,7 @@`
		`# coding: utf-8`
		`import sys`
		`+ import copy`
		`+ import jinja2`


		`# pylint: disable=R0903`
		`@@ -14,3 +16,109 @@`
		`else: # Python 2`
		`def __str__(self):`
		`return self.__unicode__().encode('utf8')`
		`+`
		`+`
		`+ mock_config_template = """\`
		`+ include('/etc/mock/{{ chroot }}.cfg')`
		`+`
		`+ config_opts['root'] = '{{ root_id }}'`
		`+ {% if buildroot_pkgs %}`
		`+ config_opts['chroot_additional_packages'] = '{{ buildroot_pkgs\| join(" ") }}'`
		`+ {% endif -%}`
		`+ {% if enable_net %}`
		`+ config_opts['rpmbuild_networking'] = True`
		`+ config_opts['use_host_resolv'] = True`
		`+ {% else %}`
		`+ config_opts['rpmbuild_networking'] = False`
		`+ config_opts['use_host_resolv'] = False`
		`+ {% endif %}`
		`+ config_opts['use_bootstrap_container'] = {{ 'True' if use_bootstrap_container else 'False' }}`
		`+ {% if use_bootstrap_container %}`
		`+ config_opts['bootstrap_chroot_additional_packages'] = []`
		`+ config_opts['bootstrap_module_enable'] = []`
		`+ config_opts['bootstrap_module_install'] = []`
		`+ {%- endif %}`
		`+ {%- if repos -%}`
		`+ config_opts['{{ pkg_manager_conf }}.conf'] += \"\"\"`
		`+ {% for repo in repos %}`
		`+ [{{ repo["id"] }}]`
		`+ name={{ repo["name"] }}`
		`+ baseurl={{ repo["url"] }}`
		`+ gpgcheck=0`
		`+ enabled=1`
		`+ skip_if_unavailable=1`
		`+ metadata_expire=0`
		`+ cost=1`
		`+ best=1`
		`+ {% endfor %}`
		`+ \"\"\"`
		`+ {%- endif -%}`
		`+ """`
		`+`
		`+ class BuildConfig(object):`
		`+ """`
		`+ build config abstraction`
		`+`
		`+ TODO: we should have some "private" library namespace (copr-common.rpm),`
		`+ which would be targeted for "internal" use-cases (backend, frontend,`
		`+ dist-git, builder, ...), not for end-user scripts.`
		`+ """`
		`+`
		`+ data = {}`
		`+`
		`+`
		`+ def __init__(self, data=None):`
		`+ self.set_item("enable_net", False)`
		`+ self.set_item("buildroot_pkgs", [])`
		`+ self.set_item("chroot", 'fedora-rawhide-x86_64')`
		`+ self.set_item("repos", [])`
		`+`
		`+ self.set_item("use_bootstrap_container", False)`
		`+`
		`+ if data:`
		`+ self.from_dict(data)`
		`+`
		`+`
		`+ def set_item(self, key, value):`
		`+ self.data[key] = copy.deepcopy(value)`
		`+`
		`+`
		`+ def from_dict(self, data):`
		`+ if 'additional_packages' in data:`
		`+ # historically 'build-config' api url defined 'additional_packages'`
		`+ # field, and 'copr-rpmbuild' renamed that to 'buildroot_pkgs'.`
		`+ # Let's support both for some time.`
		`+ data['buildroot_pkgs'] = data['additional_packages']`
		`+`
		`+ for key in data:`
		`+ self.set_item(key, data[key])`
		`+`
		`+`
		`+ def to_dict(self):`
		`+ output = copy.deepcopy(self.data)`
		`+ output['root_id'] = self.root_id()`
		`+ output['pkg_manager_conf'] = "dnf" if "custom-1" in output["chroot"] else "yum"`
		`+ return output`
		`+`
		`+`
		`+ def root_id(self):`
		`+ if 'task_id' in self.data:`
		`+ # set by /backend/get-build-task/<build_id>/`
		`+ return str(self.data['task_id'])`
		`+`
		`+ if 'project_id' in self.data:`
		`+ # set by /api/coprs/<copr>/build-config/<chroot>/`
		`+ return "{0}_{1}".format(self.data['project_id'], self.data['chroot'])`
		`+`
		`+ if 'project_name' in self.data and 'project_owner' in self.data:`
		`+ return "{0}_{1}".format(`
		`+ self.data['project_name'],`
		`+ self.data['project_owner']`
		`+ )`
		`+`
		`+ return "unset"`
		`+`
		`+`
		`+ def to_mock_config(self):`
		`+ template = jinja2.Environment().from_string(mock_config_template)`
		`+ return template.render(self.to_dict())`

rpmbuild/__init__.py

file removed

rpmbuild/copr-rpmbuild

file added

+22

		`@@ -0,0 +1,22 @@`
		`+ #! /bin/sh`
		`+`
		`+ # Run copr-rpmbuild script directly from git.`
		`+ # Copyright (C) 2017 Red Hat, Inc.`
		`+ #`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+ #`
		`+ # You should have received a copy of the GNU General Public License along`
		`+ # with this program; if not, write to the Free Software Foundation, Inc.,`
		`+ # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.`
		`+`
		`+ absdir="$(dirname "$(readlink -f "$0")")"`
		`+ export PYTHONPATH="$absdir:$absdir/../python${PYTHONPATH+:$PYTHONPATH}"`
		`+ exec python3 -m main "$@"`

rpmbuild/copr-rpmbuild.spec

file modified

+1 -2

		`@@ -23,6 +23,7 @@`
		`Requires: python3-munch`
		`Requires: python3-configparser`
		`Requires: python3-simplejson`
		`+ Requires: python3-copr`

		`Requires: mock`
		`Requires: git`
		`@@ -53,7 +54,6 @@`
		`install -d %{buildroot}%{_bindir}`
		`install -m 755 main.py %{buildroot}%{_bindir}/copr-rpmbuild`
		`install -m 644 main.ini %{buildroot}%{_sysconfdir}/copr-rpmbuild/main.ini`
		`- install -m 644 mock.cfg.j2 %{buildroot}%{_sysconfdir}/copr-rpmbuild/mock.cfg.j2`
		`install -m 644 rpkg.conf.j2 %{buildroot}%{_sysconfdir}/copr-rpmbuild/rpkg.conf.j2`
		`install -m 644 make_srpm_mock.cfg %{buildroot}%{_sysconfdir}/copr-rpmbuild/make_srpm_mock.cfg`

		`@@ -75,7 +75,6 @@`

		`%dir %{_sysconfdir}/copr-rpmbuild`
		`%config(noreplace) %{_sysconfdir}/copr-rpmbuild/main.ini`
		`- %config(noreplace) %{_sysconfdir}/copr-rpmbuild/mock.cfg.j2`
		`%config(noreplace) %{_sysconfdir}/copr-rpmbuild/rpkg.conf.j2`
		`%config(noreplace) %{_sysconfdir}/copr-rpmbuild/make_srpm_mock.cfg`

rpmbuild/copr_rpmbuild/builders/mock.py

file modified

+19 -11

		`@@ -6,6 +6,7 @@`

		`from jinja2 import Environment, FileSystemLoader`
		`from ..helpers import run_cmd, locate_spec, locate_srpm, CONF_DIRS, get_mock_uniqueext`
		`+ from copr.util import BuildConfig`

		`log = logging.getLogger("__main__")`

		`@@ -13,12 +14,16 @@`
		`class MockBuilder(object):`
		`def __init__(self, task, sourcedir, resultdir, config):`
		`self.task_id = task["task_id"]`
		`- self.chroot = task["chroot"]`
		`- self.buildroot_pkgs = task["buildroot_pkgs"]`
		`- self.enable_net = task["enable_net"]`
		`- self.repos = task["repos"]`
		`- self.use_bootstrap_container = task["use_bootstrap_container"]`
		`- self.pkg_manager_conf = "dnf" if "custom-1" in task["chroot"] else "yum"`
		`+ self.build_config = task["build_config"]`
		`+`
		`+ if 'enable_net' in task:`
		`+ # Hack! We should use dedicated options only:`
		`+ # https://bugzilla.redhat.com/show_bug.cgi?id=1513953`
		`+ # ... naturally we shouldn't touch/edit project's`
		`+ # mock-configuration per-build; that's configuration on`
		+ # per-project level; can be generated by `copr mock-config`.
		`+ self.build_config['enable_net'] = task['enable_net']`
		`+`
		`self.timeout = task["timeout"]`
		`self.sourcedir = sourcedir`
		`self.resultdir = resultdir`
		`@@ -29,8 +34,12 @@`
		`def run(self):`
		`configdir = os.path.join(self.resultdir, "configs")`
		`os.makedirs(configdir)`
		`+`
		`+ # Unused actually ATM. It is copied to result dir only for user's`
		`+ # convenience to have complete mock configuration during debugging.`
		`shutil.copy2("/etc/mock/site-defaults.cfg", configdir)`
		`shutil.copy2("/etc/mock/{0}.cfg".format(self.chroot), configdir)`
		`+`
		`cfg = self.render_config_template()`
		`with open(os.path.join(configdir, "child.cfg"), "w") as child:`
		`child.write(cfg)`
		`@@ -45,11 +54,10 @@`
		`self.produce_rpm(srpm, configdir, self.resultdir)`

		`def render_config_template(self):`
		`- jinja_env = Environment(loader=FileSystemLoader(CONF_DIRS))`
		`- template = jinja_env.get_template("mock.cfg.j2")`
		`- return template.render(chroot=self.chroot, task_id=self.task_id, buildroot_pkgs=self.buildroot_pkgs,`
		`- enable_net=self.enable_net, use_bootstrap_container=self.use_bootstrap_container,`
		`- repos=self.repos, pkg_manager_conf=self.pkg_manager_conf)`
		`+ bc = BuildConfig(self.build_config)`
		`+ # praiskup: is this needed?`
		`+ bc.set_item('task_id', self.task_id)`
		`+ return bc.to_mock_config()`

		`def preexec_fn_build_stream(self):`
		`if not self.logfile:`

rpmbuild/main.py

file modified

rpmbuild/mock.cfg.j2

file removed

-39

		`@@ -1,39 +0,0 @@`
		`- include('/etc/mock/{{ chroot }}.cfg')`
		`-`
		`- config_opts['root'] = '{{ task_id }}'`
		`-`
		`- {% if buildroot_pkgs %}`
		`- config_opts['chroot_additional_packages'] = '{{ buildroot_pkgs\| join(" ") }}'`
		`- {% endif %}`
		`-`
		`- {% if enable_net %}`
		`- config_opts['rpmbuild_networking'] = True`
		`- config_opts['use_host_resolv'] = True`
		`- {% else %}`
		`- config_opts['rpmbuild_networking'] = False`
		`- config_opts['use_host_resolv'] = False`
		`- {% endif %}`
		`-`
		`- config_opts['use_bootstrap_container'] = {{ 'True' if use_bootstrap_container else 'False' }}`
		`-`
		`- {% if use_bootstrap_container %}`
		`- config_opts['bootstrap_chroot_additional_packages'] = []`
		`- config_opts['bootstrap_module_enable'] = []`
		`- config_opts['bootstrap_module_install'] = []`
		`- {% endif %}`
		`-`
		`- {% if repos %}`
		`- config_opts['{{ pkg_manager_conf }}.conf'] += """`
		`- {% for repo in repos %}`
		`- [{{ repo["id"] }}]`
		`- name='{{ repo["name"] }}'`
		`- baseurl={{ repo["url"] }}`
		`- gpgcheck=0`
		`- enabled=1`
		`- skip_if_unavailable=1`
		`- metadata_expire=0`
		`- cost=1`
		`- best=1`
		`- {% endfor %}`
		`- """`
		`- {% endif %}`

rpmbuild/run_tests.sh

file modified

+5 -2

		`@@ -1,2 +1,5 @@`
		`- #!/bin/sh`
		`- python3 -m pytest tests -s $@`
		`+ #!/bin/sh -x`
		`+`
		`+ absdir="$(dirname "$(readlink -f "$0")")"`
		`+ export PYTHONPATH="$absdir:$absdir/../python${PYTHONPATH+:$PYTHONPATH}"`
		`+ exec python3 -m pytest tests -s "$@"`

rpmbuild/tests/test_mock.py

file modified

+59 -21

		`@@ -3,12 +3,20 @@`
		`import mock`
		`import configparser`
		`from os.path import realpath, dirname`
		`- from ..copr_rpmbuild.builders.mock import MockBuilder`
		`+ from copr_rpmbuild.builders.mock import MockBuilder`
		`import subprocess`
		`import datetime`
		`+ import copy`
		`+ from io import StringIO`

		`from unittest.mock import MagicMock`

		`+ the_repo = {`
		`+ 'id': 'copr_base',`
		`+ 'name': 'Copr repository',`
		`+ 'url': 'http://example.com/'`
		`+ }`
		`+`
		`class TestMockBuilder(unittest.TestCase):`
		`def setUp(self):`
		`self.task = {`
		`@@ -16,8 +24,6 @@`
		`"_expected_outcome": "success",`

		`"build_id": 10,`
		`- "buildroot_pkgs": ["pkg1", "pkg2", "pkg3"],`
		`- "chroot": "fedora-24-x86_64",`
		`"enable_net": True,`
		`"source_json": {`
		`"clone_url": "https://src.fedoraproject.org/git/rpms/389-admin-console.git",`
		`@@ -30,11 +36,20 @@`
		`"pkgs": "",`
		`"project_name": "copr-dev",`
		`"project_owner": "@copr",`
		`- "repos": [],`
		`"submitter": "clime",`
		`"task_id": "10-fedora-24-x86_64",`
		`"timeout": 21600,`
		`- "use_bootstrap_container": False,`
		`+`
		`+ "build_config": {`
		`+ "additional_packages": ['pkg1', 'pkg2', 'pkg3'],`
		`+ "chroot": "fedora-24-x86_64",`
		`+ "enable_net": False,`
		`+ "project_id": "praiskup-ping",`
		`+ "repos": [the_repo],`
		`+ "use_bootstrap_container": False,`
		`+ }`
		`+`
		`+`
		`}`
		`self.sourcedir = "/path/to/sourcedir"`
		`self.resultdir = "/path/to/resultdir"`
		`@@ -45,34 +60,51 @@`
		`def test_init(self):`
		`builder = MockBuilder(self.task, self.sourcedir, self.resultdir, self.config)`
		`self.assertEqual(builder.task_id, "10-fedora-24-x86_64")`
		`- self.assertEqual(builder.chroot, "fedora-24-x86_64")`
		`- self.assertEqual(builder.buildroot_pkgs, ["pkg1", "pkg2", "pkg3"])`
		`- self.assertEqual(builder.enable_net, True)`
		`- self.assertEqual(builder.repos, [])`
		`- self.assertEqual(builder.use_bootstrap_container, False)`
		`+ self.assertEqual(builder.build_config['chroot'], "fedora-24-x86_64")`
		`+ self.assertEqual(builder.build_config['additional_packages'], ["pkg1", "pkg2", "pkg3"])`
		`+ self.assertEqual(builder.build_config['enable_net'], True)`
		`+ self.assertEqual(builder.build_config['repos'], [the_repo])`
		`+ self.assertEqual(builder.build_config['use_bootstrap_container'], False)`

		`- def test_render_config_template(self):`
		`- confdirs = [dirname(dirname(realpath(__file__)))]`
		`- builder = MockBuilder(self.task, self.sourcedir, self.resultdir, self.config)`
		`- cfg = builder.render_config_template()`

		`+ def parse_mock_conf(self, cfg):`
		`# Parse the rendered config`
		`# This is how mock itself does it`
		`def include(args, *kwargs):`
		`pass`
		`- config_opts = {"yum.conf": []}`
		`+`
		`+ config_opts = {`
		`+ 'yum.conf': '',`
		`+ 'dnf.conf': '',`
		`+ }`
		`cfg = re.sub(r'include$(.*)$', r'include(\g<1>, config_opts, True)', cfg)`
		`code = compile(cfg, "/tmp/foobar", 'exec')`
		`exec(code)`
		`+ return config_opts`
		`+`
		`+`
		`+ def assert_repo_content(self, config):`
		`+ buf = StringIO(config)`
		`+ config = configparser.ConfigParser()`
		`+ config.readfp(buf)`
		`+ self.assertEqual(config.get(the_repo['id'], 'baseurl'), the_repo['url'])`
		`+ self.assertEqual(config.get(the_repo['id'], 'name'), the_repo['name'])`

		`+`
		`+ def test_render_config_template(self):`
		`+ confdirs = [dirname(dirname(realpath(__file__)))]`
		`+ builder = MockBuilder(self.task, self.sourcedir, self.resultdir, self.config)`
		`+ cfg = builder.render_config_template()`
		`+ config_opts = self.parse_mock_conf(cfg)`
		`self.assertEqual(config_opts["root"], "10-fedora-24-x86_64")`
		`self.assertEqual(config_opts["chroot_additional_packages"], "pkg1 pkg2 pkg3")`
		`self.assertEqual(config_opts["rpmbuild_networking"], True)`
		`self.assertEqual(config_opts["use_bootstrap_container"], False)`
		`- self.assertEqual(config_opts["yum.conf"], [])`
		`+ self.assert_repo_content(config_opts['yum.conf'])`
		`+`

		`- @mock.patch("rpmbuild.copr_rpmbuild.builders.mock.get_mock_uniqueext")`
		`- @mock.patch("rpmbuild.copr_rpmbuild.builders.mock.subprocess.Popen")`
		`+ @mock.patch("copr_rpmbuild.builders.mock.get_mock_uniqueext")`
		`+ @mock.patch("copr_rpmbuild.builders.mock.subprocess.Popen")`
		`def test_produce_rpm(self, popen_mock, get_mock_uniqueext_mock):`
		`builder = MockBuilder(self.task, self.sourcedir, self.resultdir, self.config)`
		`get_mock_uniqueext_mock.return_value = '2'`
		`@@ -95,6 +127,12 @@`

		`def test_custom1_chroot_settings(self):`
		`b1 = MockBuilder(self.task, self.sourcedir, self.resultdir, self.config)`
		`- b2 = MockBuilder(dict(self.task, **{"chroot": "custom-1-x86_64"}), self.sourcedir, self.resultdir, self.config)`
		`- self.assertEqual(b1.pkg_manager_conf, "yum")`
		`- self.assertEqual(b2.pkg_manager_conf, "dnf")`
		`+ task2 = copy.deepcopy(dict(self.task))`
		`+ task2['build_config']['chroot'] = 'custom-1-x86_64'`
		`+ b2 = MockBuilder(task2, self.sourcedir, self.resultdir, self.config)`
		`+ cfg1 = b1.render_config_template()`
		`+ cfg2 = b2.render_config_template()`
		`+ cfg1 = self.parse_mock_conf(cfg1)`
		`+ cfg2 = self.parse_mock_conf(cfg2)`
		`+ self.assert_repo_content(cfg1['yum.conf'])`
		`+ self.assert_repo_content(cfg2['dnf.conf'])`

rpmbuild/tests/test_providers.py

file modified

+2 -2

		`@@ -1,8 +1,8 @@`
		`import unittest`
		`- from ..copr_rpmbuild.providers import (factory, RubyGemsProvider, PyPIProvider,`
		`+ from copr_rpmbuild.providers import (factory, RubyGemsProvider, PyPIProvider,`
		`SpecUrlProvider)`

		`- from ..copr_rpmbuild.helpers import SourceType`
		`+ from copr_rpmbuild.helpers import SourceType`


		`class TestProvidersFactory(unittest.TestCase):`

rpmbuild/tests/test_pypi.py

file modified

+2 -2

		`@@ -1,6 +1,6 @@`
		`import mock`
		`import unittest`
		`- from ..copr_rpmbuild.providers.pypi import PyPIProvider`
		`+ from copr_rpmbuild.providers.pypi import PyPIProvider`


		`class TestPyPIProvider(unittest.TestCase):`
		`@@ -16,7 +16,7 @@`
		`self.assertEqual(provider.pypi_package_name, "motionpaint")`
		`self.assertEqual(provider.python_versions, [2, 3])`

		`- @mock.patch("rpmbuild.copr_rpmbuild.providers.pypi.run_cmd")`
		`+ @mock.patch("copr_rpmbuild.providers.pypi.run_cmd")`
		`@mock.patch("builtins.open")`
		`def test_produce_srpm(self, mock_open, run_cmd):`
		`provider = PyPIProvider(self.source_json, outdir="/some/tmp/directory")`

rpmbuild/tests/test_rubygems.py

file modified

+3 -3

		`@@ -1,7 +1,7 @@`
		`import mock`
		`import unittest`
		`from munch import Munch`
		`- from ..copr_rpmbuild.providers.rubygems import RubyGemsProvider`
		`+ from copr_rpmbuild.providers.rubygems import RubyGemsProvider`


		`class TestRubyGemsProvider(unittest.TestCase):`
		`@@ -13,7 +13,7 @@`
		`provider = RubyGemsProvider(self.source_json, self.resultdir)`
		`self.assertEqual(provider.gem_name, "A_123")`

		`- @mock.patch("rpmbuild.copr_rpmbuild.providers.rubygems.run_cmd")`
		`+ @mock.patch("copr_rpmbuild.providers.rubygems.run_cmd")`
		`@mock.patch("builtins.open")`
		`def test_produce_srpm(self, mock_open, run_cmd):`
		`provider = RubyGemsProvider(self.source_json, self.resultdir)`
		`@@ -21,7 +21,7 @@`
		`assert_cmd = ["gem2rpm", "A_123", "--srpm", "-C", "/path/to/resultdir", "--fetch"]`
		`run_cmd.assert_called_with(assert_cmd)`

		`- @mock.patch("rpmbuild.copr_rpmbuild.providers.rubygems.run_cmd")`
		`+ @mock.patch("copr_rpmbuild.providers.rubygems.run_cmd")`
		`@mock.patch("builtins.open")`
		`def test_empty_license(self, mock_open, run_cmd):`
		`stderr = ("error: line 8: Empty tag: License:"`

rpmbuild/tests/test_scm.py

file modified

+5 -5

		`@@ -3,8 +3,8 @@`
		`import tempfile`
		`import os`
		`import configparser`
		`- from ..copr_rpmbuild.providers.scm import ScmProvider`
		`- from ..copr_rpmbuild.helpers import read_config`
		`+ from copr_rpmbuild.providers.scm import ScmProvider`
		`+ from copr_rpmbuild.helpers import read_config`

		`from mock import patch, MagicMock`

		`@@ -85,7 +85,7 @@`
		`source_json = self.source_json.copy()`
		`source_json["clone_url"] = "http://copr-dist-git.fedorainfracloud.org/git/clime/project/pkg.git"`

		`- with patch("rpmbuild.copr_rpmbuild.providers.scm.CONF_DIRS", new=[rpkg_tmpdir]):`
		`+ with patch("copr_rpmbuild.providers.scm.CONF_DIRS", new=[rpkg_tmpdir]):`
		`provider = ScmProvider(source_json, self.resultdir, self.config)`
		`rpkg_config_path = provider.generate_rpkg_config()`

		`@@ -97,7 +97,7 @@`

		`source_json["clone_url"] = "http://unknownurl/git/clime/project/pkg.git"`

		`- with patch("rpmbuild.copr_rpmbuild.providers.scm.CONF_DIRS", new=[rpkg_tmpdir]):`
		`+ with patch("copr_rpmbuild.providers.scm.CONF_DIRS", new=[rpkg_tmpdir]):`
		`provider = ScmProvider(source_json, self.resultdir, self.config)`
		`rpkg_config_path = provider.generate_rpkg_config()`
		`self.assertEqual(rpkg_config_path, "/etc/rpkg.conf")`
		`@@ -119,7 +119,7 @@`
		`assert_cmd = ["tito", "build", "--test", "--srpm", "--output", self.resultdir]`
		`self.assertEqual(provider.get_tito_test_command(), assert_cmd)`

		`- @mock.patch("rpmbuild.copr_rpmbuild.providers.scm.get_mock_uniqueext")`
		`+ @mock.patch("copr_rpmbuild.providers.scm.get_mock_uniqueext")`
		`def test_get_make_srpm_command(self, get_mock_uniqueext_mock):`
		`get_mock_uniqueext_mock.return_value = '2'`
		`provider = ScmProvider(self.source_json, self.resultdir, self.config)`

rpmbuild/tests/test_spec.py

file modified

+2 -2

		`@@ -1,7 +1,7 @@`
		`import mock`
		`import unittest`
		`import configparser`
		`- from ..copr_rpmbuild.providers.spec import SpecUrlProvider`
		`+ from copr_rpmbuild.providers.spec import SpecUrlProvider`


		`class TestSpecUrlProvider(unittest.TestCase):`
		`@@ -14,7 +14,7 @@`
		`self.assertEqual(provider.url, "http://foo.ex/somepackage.spec")`

		`@mock.patch('requests.get')`
		`- @mock.patch("rpmbuild.copr_rpmbuild.providers.spec.run_cmd")`
		`+ @mock.patch("copr_rpmbuild.providers.spec.run_cmd")`
		`@mock.patch('builtins.open', new_callable=mock.mock_open())`
		`def test_produce_srpm(self, mock_open, run_cmd, mock_get):`
		`provider = SpecUrlProvider(self.source_json, self.resultdir)`

praiskup commented 6 years ago

From now on, copr-cli mock-config and copr-rpmbuild use the
same mock configuration file (both use the same methods for
generating the mock configuration).

This is done with believe that mock configuration should be
"cache"-able thing -- so that it can be generated on client side
once per copr project, and then re-used to build sets of packages
without re-generating/changing the config file all over again.
There's one hack with 'enable_net' which breaks this concept,
though.

praiskup commented 6 years ago

WIP of course, I'd like to discuss this carefully.

clime commented 6 years ago

Thank you for the PR.

I understand that you would like to unify the mock config generation for copr-cli and copr-rpmbuild. Normally, such unification would be welcome but not exactly here as copr-cli mock-config was a mistake that we should not further develop upon.

It was put there simply because we lacked a better place at the time, which is copr-rpmbuild - tool responsible for actual building that needs to be concerned with the actual build configuration and it is quite natural for it to be able to print the generated configs without building anything.

Other argument is that python-copr is a library that enables users to easily communicate with copr-frontend API (in other words, it is a high-level abstraction over the API) and should not be mis-used to do anything else (e.g. generating mock configuration files). I was seeing copr-cli mock-config as a potentially useful hack that should not really be there and now it's the time to fix it when we actually have the right place where to put that functionality.

To sum up, much better approach here is to move the functionality of generating mock configs solely into copr-rpmbuild and implement something like copr-rpmbuild --rpm --build-config to print the to-be-used mock config to stdout. copr-cli mock-config will be deprecated at the same time. Thank you if you will take on this.

praiskup commented 6 years ago

I understand that you would like to unify the mock config generation for copr-cli and copr-rpmbuild. Normally, such unification would be welcome but not exactly here as copr-cli mock-config was a mistake that we should not further develop upon.

I must say I'm sad to hear this, I'm not really sure what's wrong on copr mock-config functionality. That is in total sync with what koji/brew does... and what packager want's to do (work with mock.., not with copr-rpmbuild). Speaking about mistakes (mostly mine in this case, as the original author) reading the copr-rpmbuild code I'm sure the mistake was that originally copr-rpmbuild re-implemented the mock configuration generator once more, while there was one already :-( ready to take. Another mistake was that I was forced to implement build-config json as output from frontend -- I thought (and I still think) that the mock config should have been generated directly by frontend :-(.

It was put there simply because we lacked a better place at the time, which is copr-rpmbuild - tool responsible for actual building that needs to be concerned with the actual build configuration and it is quite natural for it to be able to print the generated configs without building anything.

Which this contribution totally aligns with...

Other argument is that python-copr is a library that enables users to easily communicate with copr-frontend API (in other words, it is a high-level abstraction over the API) and should not be mis-used to do anything else

What else the mock configuration generator is than API for developing packages? OTOH, I agree that better would be to have it only as cli API than python API.. I agree that the python api is basically useless for user, is this the issue?

To sum up, much better approach here is to move the functionality of generating mock configs solely into copr-rpmbuild and implement something like copr-rpmbuild --rpm --build-config to print the to-be-used mock config to stdout. copr-cli mock-config will be deprecated at the same time. Thank you if you will take on this.

I can not disagree more, copr-rpmbuild implements that much stuff that which is terribly hard to contribute to :-( been there last week :-(. I was thinking dozen of times about how it puts obstacles to use mock directly; and how it kills the original idea behind "copr is just thin layer above mock".

I agree that there could be copr-rpmbuild --build-config something, but why not to reuse some "internal library" code? copr-rpmbuild is not meant to be /bin/copr replacement; more it is not mean to be other than debugging helper AND glue between backend and builder (tool to easily prepare builder by the package installation).

Some idea I was thinking about during last years .. would python-copr-common package help? The package would be used to share library code among frontend, backend, distgit, and I could put the mock-config generator there... That package would be basically unusable for the end-user, so we don't have to keep the API stable..?

Well, there's some idea and belief behind this contribution :-) and I'm kindly hoping for reconsideration.. so I can update this WIP PoC and continue, rather than scratch this.

clime commented 6 years ago

I must say I'm sad to hear this, I'm not really sure what's wrong on copr mock-config functionality. That is in total sync with what koji/brew does... and what packager want's to do (work with mock.., not with copr-rpmbuild).

koji or brew do not need to be necessarily correct about everything. copr-rpmbuild is not a known tool currently but not sure why a packager wouldn't work with it when the implementation is ready. It should provide an easier way to build directly from SRPM URL, Git repo, etc. (see comments in https://pagure.io/copr/copr/issue/102)

copr-rpmbuild re-implemented the mock configuration generator once more, while there was one already :-( ready to take.

Well, you would need dependency on copr-cli just because of that. That doesn't make much sense as copr-cli brings much of other deps. And again, copr-cli is a tool to communicate with copr-frontend API whereas copr-rpmbuild uses just internal interface dedicated for backend. It's a nice separation and I don't see a reason why to break it.

Another mistake was that I was forced to implement build-config json as output from frontend -- I thought (and I still think) that the mock config should have been generated directly by frontend :-(.

Sorry but that's another thing I would like to maintain in the code and that is that frontend only handles build configuration on higher abstraction level - like 'networking enabled', 'additional repos', 'buildroot packages', etc. and not on the level of the concrete technology. The original authors intended it, otherwise they would just allow uploading mock configuration to perform the build. I want to respect the decision of the original authors and keep this paradigm.

I can not disagree more, copr-rpmbuild implements that much stuff that which is terribly hard to contribute to :-( been there last week :-(. I was thinking dozen of times about how it puts obstacles to use mock directly; and how it kills the original idea behind "copr is just thin layer above mock".

Well, I guess it's hard to contribute to it for you because you have very different ideas about how COPR development should look like. It would be useful to sync in this but I don't think that will ever happen. I don't see much obstacles in copr-rpmbuild. And yeah, copr was originally designed as thin layer above mock but now that's not so much true anymore. And I don't think it needs to be. I don't see why we should make tighter connection to particular technology when we don't need to.

I agree that there could be copr-rpmbuild --build-config something, but why not to reuse some "internal library" code? copr-rpmbuild is not meant to be /bin/copr replacement; more it is not mean to be other than debugging helper AND glue between backend and builder (tool to easily prepare builder by the package installation).

What I am saying is that python-copr is not a very good place for that code as python-copr so far just implements just python version of the API (it just wraps API calls) and I would really like to keep it like that. Otherwise I agree with what is said in this paragraph.

Some idea I was thinking about during last years .. would python-copr-common package help? The package would be used to share library code among frontend, backend, distgit, and I could put the mock-config generator there... That package would be basically unusable for the end-user, so we don't have to keep the API stable..?

python-copr-common would be a better place for sure.

msuchy commented 6 years ago

My $0.02:

I think that mock config should be generated by copr-backend. Only backend knows what version of mock and what features it is used. It would be nice if it would be small script or function, which generate it from template the way you created it.
This script can go to copr-backend-mockconfig subpackage, which can be used on client as well.

I agree with with @clime that this should likely not go to copr-cli or python-copr.

clime commented 6 years ago

I think that mock config should be generated by copr-backend. ...

Well, we have been there and moved to generating code by a builder script, now its copr-rpmbuild which has the mock dependency and can operate with it (certain version of copr-rpmbuild might require certain version of mock that supports certain configuration arguments). This is something we cannot do if the template is moved to a library that will not (typically) have the mock dependency. It could be solved via 'Provides' (i.e. Provides: mock-conf-1.4.4) in the library although that might not be that intuitive. Possible though...

praiskup commented 6 years ago

We all agree that python-copr is improper place for this code then. I hear nothing against say python-copr-internal (sub)package (previously spelled python-copr-common? Then, copr-cli and copr-rpmbuild can use this basically for free and keep all the benefits..

@clime wrote:

Well, you would need dependency on copr-cli just because of that. That doesn't make much sense as copr-cli brings much of other deps.

Agreed, that's why I'm trying to find better place than copr-cli. Underlining once more, python-copr is neither the right place.. (it is just a bit better than copr-cli).

And again, copr-cli is a tool to communicate with copr-frontend API whereas copr-rpmbuild uses just internal interface dedicated for backend. It's a nice separation and I don't see a reason why to break it.

Agreed, and I believe there's nothing against this in the WIP code.

Well, I guess it's hard to contribute to it for you because you have very different ideas about how COPR development should look like.

It's not really development, even though comparing to other upstreams I have some remarks. The thing is that I only can administrate and contribute to projects which I believe they have clear and sure future; for that I need to see that there's clear (and "finite") architecture goal. If I feel that something goes against the goal/vision, I naturally try to polish that to keep myself motivated, etc. (you probably know that it is mostly volunteering effort from my side, and I want to be honest there, this PR was the hardest and the most expensive one so far for me..). There's nothing fishy, really...

@msuchy wrote:

Only backend knows what version of mock and what features it is used.

Not anymore I think. The thing is that backend actually doesn't even implicitly know the mock version installed on builder, and frontend is in the same position (but frontend has -- without any implementation details and API calls -- all the actual build info needed to generate the so far trivial mock config). We are complicating our life IMO (the generated mock profile could document that it is useful with mock >= some_version), and could be done solely on frontend). Nevermind though if we move the generator to some central place... (then it doesn't matter who calls the internal-library call..)

@clime

This is something we cannot do if the template is moved to a library that will not (typically) have the mock dependency.

Even though I think we are solving some hypothetical issue, the library package can use Conflicts: mock <= the-version.

clime commented 6 years ago

@clime

This is something we cannot do if the template is moved to a library that will not (typically) have the mock dependency.

Even though I think we are solving some hypothetical issue, the library package can use Conflicts: mock <= the-version.

Perhaps, just note that if you just place that functionality into copr-rpmbuild, there is no such problem even because there you can easily require the specific version of mock explicitly because it is already in Requires. Having it in library that somehow Requires or Conflicts mock package (which is a beast on completely different system level) is a bit strange. It's a hypotetical problem but it's also quite a good indicator how things should be laid out. We seemed to have agreed that copr-rpmbuild is the place to provide that functionality. I think people that are used to copr-cli mock-config can understand the functionality has shifted somewhere else where it is more approapriate. Btw. not sure why python-copr-internal. python-copr-common seems to be more fitting for something that contains common set of utilities.

clime commented 6 years ago

But hey I am happy about you contributing. It would help to better coordinate but still, thanks for all the feedback!

praiskup commented 6 years ago

Having it in library that somehow Requires or Conflicts mock package (which is a beast on completely different system level) is a bit strange.

In different wording; I don't think that this all is needed.. I'm just saying it is possible. If some library generates some mock config, it doesn't necessarily mean that the config needs to be use-able on the target system. You are just asking to get the config which is used in copr buildsystem, and you've got it (it's up to you to get appropriate version of the mock tool, e.g. by the given comment).

We seemed to have agreed that copr-rpmbuild is the place to provide that functionality.

agree that copr-rpmbuild can provide this, too (same as it could just take the config from frontend/backend).

I think people that are used to copr-cli mock-config can understand the functionality has shifted somewhere else where it is more approapriate.

On this I really dislike the need to install one more tool just to get the mock config. Same as I would really dislike movement when there's a solution without compromises.

Btw. not sure why python-copr-internal. python-copr-common seems to be more fitting for something that contains common set of utilities.

Ah didn't say the reason, I wanted to make clear statement that end-users shouldn't depend on this library; but yeah, perhaps python-copr-common is self-explaining too.. ack.

clime commented 6 years ago

In different wording; I don't think that this all is needed.. I'm just saying it is possible. If some library generates some mock config, it doesn't necessarily mean that the config needs to be use-able on the target system. You are just asking to get the config which is used in copr buildsystem, and you've got it (it's up to you to get appropriate version of the mock tool, e.g. by the given comment).

I guess it boils down to a question: What do you need to reproduce a certain build locally.

With config generator in copr-rpmbuild, you need to use the same version of mock and copr-rpmbuild as COPR used at the time of the build.

With config generator in the library, you need the same version of mock, copr-rpmbuild, and the library to reproduce the build (you need copr-rpmbuild because it calls mock with certain command-line arguments, does the job around cloning the repo, etc.).

By placing this code in a library, you are introducing one more possible point of failure where user has the same version of copr-rpmbuild and mock (which copr-rpmbuild may make sure of) as COPR is currently using and expects the build to run the same (except for updates). But due to a different version of that library (possibly outdated), he will get a different mock config and possibly a different build result.

Yes, copr-rpmbuild now may ensure (and should) it has the right version of the library for the installed mock version but how if the library doesn't explicitly state it? It might state it with Provides but this whole thing just introduces a maintenance cost for not particularly clear benefits.

Please, reconsider this once more.

praiskup commented 6 years ago

Warning warning, this "reproducible" approach is totally overestimated. I'm rather
interested in simplifying development of packages.

This is ~99% truth, and matches the real use-cases I've ever done during packaging. Packager

almost never reproduce succeeded builds locally, but almost always only failed builds
even when "reproducing" build success/failure, he doesn't expect local build environment is 100% compatible with the cloud environment

For those reasons, I don't share your worry about usefulness of mock profile generator;
anyone who will hit copr mock-config does some best-effort work, and won't blame
you that his mock does something different then in copr cloud (because disabled
tmpfs in mock, systemd-nspawn unavailability, less memory or CPU available, different
timeout, etc.).

If we really want to speak about reproducing the builds, we should concentrate on
different thing -> allocating builder for user for general purpose task:
- user can not reproduce build of RPM locally...
- user contacts copr administrator with ssh public key
- copr administrators presses magic button, get's an IP and forwards that to user

Yes, copr-rpmbuild now may ensure (and should) it has the right version of the library for the installed mock version but how if the library doesn't explicitly state it? It might state it with Provides but this whole thing just introduces a maintenance cost for not particularly clear benefits.

I agree that copr-rpmbuild, because it is mainly for copr builder purposes, needs to ensure proper version of mock and version of "the library". But that's only about mentioning proper version in 'Requries:'. I may underestimate this -> so if you think so, we can "C&P" the code with generator from rpmbuild to cli during release process ... (to not duplicate something), but that's implementation detail for solution to the problem -> duplicated generator. As we discussed for a very long time, I'm OK to solve this problem the way you request it. Really, if python-copr-common is something we are OK to try, please let me finish the proposal :-).

clime commented 6 years ago

I agree that copr-rpmbuild, because it is mainly for copr builder purposes, needs to ensure proper version of mock and version of "the library". But that's only about mentioning proper version in 'Requries:'.

The library should explicitly state for which mock version it provides the config. If not and incompatible library version and mock are used together, things might work but give different results without a user noticing (e.g. in case of introduction of new boostrap_additional_packages configuration option). Therefore, from code and also user perspective, it's better to leave the generator in copr-rpmbuild and provide --build-config there. At the same time, deprecate copr-cli mock-config. Implementation of this will be welcome.

praiskup commented 6 years ago

The status now == scratching all the discussion so far, right?

praiskup commented 6 years ago

If copr-rpmbuild states which version of mock and library is needed, I don't know how this is related to copr-cli mock-config deprecation and how user will benefit from that.

Edited 6 years ago by praiskup

clime commented 6 years ago

If copr-rpmbuild states which version of mock and library is needed, I don't know how this is related to copr-cli mock-config deprecation and how user will benefit from that.

You need to know for what mock version the library provides the config. Comment in the code is really not sufficient because it can get easily outdated. If one programmer makes the change in the library and releases a new version of that library for the latest released version of mock, he should then also update copr-rpmbuild to require that version of library and that version of (the released) mock before next release of copr-rpmbuild.

So you are doing changes in one part of code but as a consequence you need to (you should) make changes in completely different part of code when there is no explicit relation from the library where you need to make the change to the copr-rpmbuild package where you should make the change. So a new programmer wouldn't know that and simply wouldn't do it.

This problem does not occur at all if the template is placed directly in the copr-rpmbuild package where it should be placed because copr-rpmbuild is the only system component that really needs it. copr-rpmbuild could provide --build-config to let a user debug mock manually if needed.

To me it is quite clear that you trying to push something, which is not a good code practice and I think we should keep good code practices in our code because otherwise we will start having something that is difficult to change because of some implicit links from one part of code to another. I really want to keep good code practices in the code so that other programmers that will work on COPR have relatively easy work.

If I didn't persuade and you are unwilling to move the functionality to the right place (or leave it there more precisely), then I cannot really do much about it. As a user, you have rights to demand for copr-cli mock-config to be kept and then the unification is better than nothing.

praiskup commented 6 years ago

To me it is quite clear that you trying to push something, which is not a good code practice

I know it is personal (said "To me...") POV, but still it is pretty offending and self-confident.

Dunno what changed since the meeting, where we discussed good/bad practices,
came to the compromise, where "build-requires/conflicts" in rpmbuilder were considered
mostly non-problematic (and are mostly theoretical, "what if mock changed, etc."). We also
agreed the python-copr-common is mostly wanted feature, not only for this particular
use-case and it would make sense if somebody took a look at this anyway.. (so to me, even
if this was the only benefit of this PR (it's not!) it would be worth the never-ending talk).

If I didn't persuade and you are unwilling to move the functionality to the right place (or leave it there more precisely), then I cannot really do much about it. As a user, you have rights to demand for copr-cli mock-config to be kept and then the unification is better than nothing.

I can willingly move the code were you think it is more appropriate, but yes..
... I want to keep copr mock-config feature (and ideally I want to generate mock profile
on one more place (#185)). So I want to generate it on one place actually.
Removing that user command only for such trivial implementation detail would be just
unacceptable. There are different ways how to share code across packages, just pick one
please.

Edited 6 years ago by praiskup

clime commented 6 years ago

Then, please, prepare the code so that it can be reviewed.

clime commented 6 years ago

Closing this for inactivity and also for not really reaching a general consensus. Feel free to reopen but I would like see an actual use-case for copr mock-config. I do not understand its purpose and I would like to hear it if there is one.

Pull-Request has been closed by clime

6 years ago

praiskup commented 6 years ago

Closing this for inactivity and also for not really reaching a general consensus.

Its impossible ...

Feel free to reopen but I would like see an actual use-case for copr mock-config. I do not understand its purpose and I would like to hear it if there is one.

... since if you still don't know what the copr mock-config is for, you don't listen. The problem is also probably lack of packaging experience to understand how mock --shell feature is important (every-day thing for any moderately complicated package).

PRs can not be reopened, you know it. So let's fight with wind mils once we have
python-copr-common package.