#11 Arbitrary dist-git branching
Merged 6 years ago by praiskup. Opened 7 years ago by praiskup.
Unknown source dist-git-branches-slash  into  master

@@ -144,16 +144,6 @@

              return path

      return default

  

- def chroot_to_branch(chroot):

-     """

-     Get a git branch name from chroot. Follow the fedora naming standard.

-     """

-     os_name, version, _ = chroot.split("-")

-     if os_name == "fedora":

-         os_name = "f"

-     elif os_name == "epel" and int(version) <= 6:

-         os_name = "el"

-     return "{}{}".format(os_name, version)

  

  class BackendConfigReader(object):

      def __init__(self, config_file=None, ext_opts=None):

@@ -672,7 +672,14 @@

              self.teardown_per_task_logging(per_task_log_handler)

  

      def setup_per_task_logging(self, task):

-         handler = logging.FileHandler(os.path.join(self.opts.per_task_log_dir, "{0}.log".format(task.task_id)))

+         # Avoid putting logs into subdirectories when dist git branch name

+         # contains slashes.

+         task_id = str(task.task_id).replace('/', '_')

+ 

+         handler = logging.FileHandler(

+             os.path.join(self.opts.per_task_log_dir,

+                          "{0}.log".format(task_id))

+         )

          handler.setLevel(logging.DEBUG)

          logging.getLogger('').addHandler(handler)

          return handler
@@ -796,6 +803,8 @@

              sandbox(dst_dir)

              dcmd = ["docker", "run"]

              full_name = "-".join([name or "copr", hashlib.md5().hexdigest()])

+             # For dist-git branches containing slashes.

+             full_name = full_name.replace('/', '_')

  

              if sys_admin:

                  dcmd.extend(["--cap-add=SYS_ADMIN"])

@@ -0,0 +1,52 @@

+ """map mock croots to dits-git branch

+ 

+ Revision ID: bf4b5dc74740

+ Revises: 38ea34def9a

+ Create Date: 2017-05-19 07:55:05.743045

+ 

+ """

+ 

+ # revision identifiers, used by Alembic.

+ revision = 'bf4b5dc74740'

+ down_revision = '38ea34def9a'

+ 

+ from alembic import op

+ import sqlalchemy as sa

+ 

+ from sqlalchemy.orm import sessionmaker

+ from coprs.models import MockChroot, DistGitBranch

+ from coprs.helpers import chroot_to_branch

+ from coprs.logic.coprs_logic import BranchesLogic

+ 

+ def upgrade():

+     bind = op.get_bind()

+     Session = sessionmaker()

+     session = Session(bind=bind)

+ 

+     op.create_table('dist_git_branch',

+         sa.Column('name', sa.String(length=50), nullable=False),

+         sa.PrimaryKeyConstraint('name')

+     )

+ 

+     # Nullable at this point.

+     op.add_column(u'mock_chroot', sa.Column('distgit_branch_name', sa.String(length=50), nullable=True))

+     op.create_foreign_key(None, 'mock_chroot', 'dist_git_branch', ['distgit_branch_name'], ['name'])

+ 

+     for chroot in session.query(MockChroot).all():

+         # Pick the predefined default.

+         branch = chroot_to_branch(chroot.name)

+         chroot.distgit_branch = BranchesLogic.get_or_create(branch, session)

+         session.add(chroot.distgit_branch)

+         session.add(chroot)

+ 

+     session.commit()

+ 

+     # not nulllable since now..

+     op.alter_column('mock_chroot', 'distgit_branch_name',

+                existing_type=sa.VARCHAR(length=50),

+                nullable=False)

+ 

+ 

+ def downgrade():

+     op.drop_column(u'mock_chroot', 'distgit_branch_name')

+     op.drop_table('dist_git_branch')

@@ -232,30 +232,6 @@

      return "{}{}".format(os, version)

  

  

- def branch_to_os_version(branch):

-     os = None

-     version = None

-     if branch == "master":

-         os = "fedora"

-         version = "rawhide"

-     elif branch[0] == "f":

-         os = "fedora"

-         version = branch[1:]

-     elif branch[:4] == "epel" or branch[:2] == "el":

-         os = "epel"

-         version = branch[-1:]

-     elif branch[:6] == "custom":

-         os = "custom"

-         version = branch[-1:]

-     elif branch[:3] == "mga":

-         os = "mageia"

-         version = branch[3:]

-     elif branch[:8] == "cauldron":

-         os = "mageia"

-         version = "cauldron"

-     return os, version

- 

- 

  def splitFilename(filename):

      """

      Pass in a standard style rpm fullname

@@ -638,13 +638,15 @@

          task_id consists of a name of git branch + build id

          Example: 42-f22 -> build id 42, chroots fedora-22-*

          """

-         build_id, branch = task_id.split("-")

+         build_id, branch = task_id.split("-", 1)

          build = cls.get_by_id(build_id).one()

          build_chroots = build.build_chroots

-         os, version = helpers.branch_to_os_version(branch)

-         chroot_halfname = "{}-{}".format(os, version)

-         matching = [ch for ch in build_chroots if chroot_halfname in ch.name]

-         return matching

+ 

+         # What chroots this branch is for?

+         branch_chroots = models.DistGitBranch.query.get(branch).chroots

+         branch_chroots = [x.name for x in branch_chroots]

+ 

+         return [ch for ch in build_chroots if ch.name in branch_chroots]

  

  

      @classmethod

@@ -430,6 +430,19 @@

         active_history=True, retval=False)

  

  

+ class BranchesLogic(object):

+     @classmethod

+     def get_or_create(cls, name, session=db.session):

+         item = session.query(models.DistGitBranch).filter_by(name=name).first()

+         if item:

+             return item

+ 

+         branch = models.DistGitBranch()

+         branch.name = name

+         session.add(branch)

+         return branch

+ 

+ 

  class CoprChrootsLogic(object):

      @classmethod

      def mock_chroots_from_names(cls, names):

@@ -764,6 +764,15 @@

          return result

  

  

+ class DistGitBranch(db.Model, helpers.Serializer):

+     """

+     1:N mapping: branch -> chroots

+     """

+ 

+     # Name of the branch used on dist-git machine.

+     name = db.Column(db.String(50), primary_key=True)

+ 

+ 

  class MockChroot(db.Model, helpers.Serializer):

  

      """
@@ -782,6 +791,14 @@

      arch = db.Column(db.String(50), nullable=False)

      is_active = db.Column(db.Boolean, default=True)

  

+     # Reference branch name

+     distgit_branch_name  = db.Column(db.String(50),

+                                      db.ForeignKey("dist_git_branch.name"),

+                                      nullable=False)

+ 

+     distgit_branch = db.relationship("DistGitBranch",

+             backref=db.backref("chroots"))

+ 

      @property

      def name(self):

          """
@@ -942,7 +959,7 @@

  

      @property

      def import_task_id(self):

-         return "{}-{}".format(self.build_id, helpers.chroot_to_branch(self.name))

+         return "{}-{}".format(self.build_id, self.mock_chroot.distgit_branch_name)

  

      @property

      def dist_git_url(self):
@@ -961,7 +978,7 @@

      def import_log_url(self):

          if app.config["COPR_DIST_GIT_LOGS_URL"]:

              return "{}/{}.log".format(app.config["COPR_DIST_GIT_LOGS_URL"],

-                                       self.import_task_id)

+                                       self.import_task_id.replace('/', '_'))

          return None

  

      @property

@@ -40,7 +40,7 @@

              "task_id": task.import_task_id,

              "user": copr.owner_name, # TODO: user -> owner

              "project": task.build.copr.name,

-             "branch": helpers.chroot_to_branch(task.mock_chroot.name),

+             "branch": task.mock_chroot.distgit_branch_name,

              "source_type": task.build.source_type,

              "source_json": task.build.source_json,

          }
@@ -152,7 +152,7 @@

                  "enable_net": task.build.enable_net,

                  "git_repo": task.build.package.dist_git_repo,

                  "git_hash": task.git_hash,

-                 "git_branch": helpers.chroot_to_branch(task.mock_chroot.name),

+                 "git_branch": task.mock_chroot.distgit_branch_name,

                  "package_name": task.build.package.name,

                  "package_version": task.build.pkg_version

              }

@@ -20,6 +20,7 @@

  from coprs.whoosheers import CoprWhoosheer

  from run import generate_repo_packages

  from sqlalchemy import or_

+ from coprs.helpers import chroot_to_branch

  

  

  class TestCommand(Command):
@@ -118,10 +119,21 @@

  

      "Creates a mock chroot in DB"

  

-     def run(self, chroot_names):

+     def __init__(self):

+         self.option_list += Option(

+                "--dist-git-branch",

+                "-b",

+                dest="branch",

+                help="Branch name for this set of new chroots"),

+ 

+     def run(self, chroot_names, branch=None):

          for chroot_name in chroot_names:

+             if not branch:

+                 branch = chroot_to_branch(chroot_name)

+             branch_object = coprs_logic.BranchesLogic.get_or_create(branch)

              try:

-                 coprs_logic.MockChrootsLogic.add(chroot_name)

+                 chroot = coprs_logic.MockChrootsLogic.add(chroot_name)

+                 chroot.distgit_branch = branch_object

                  db.session.commit()

              except exceptions.MalformedArgumentException:

                  self.print_invalid_format(chroot_name)

@@ -15,6 +15,7 @@

  

  from coprs import helpers

  from coprs import models

+ from coprs.logic.coprs_logic import BranchesLogic

  

  import six

  from coprs.helpers import StatusEnum
@@ -148,12 +149,19 @@

      def f_mock_chroots(self):

          self.mc1 = models.MockChroot(

              os_release="fedora", os_version="18", arch="x86_64", is_active=True)

+         self.mc1.distgit_branch = models.DistGitBranch(name='f18')

+ 

          self.mc2 = models.MockChroot(

              os_release="fedora", os_version="17", arch="x86_64", is_active=True)

+         self.mc2.distgit_branch = models.DistGitBranch(name='fedora-17')

+ 

          self.mc3 = models.MockChroot(

              os_release="fedora", os_version="17", arch="i386", is_active=True)

+         self.mc3.distgit_branch = self.mc2.distgit_branch

+ 

          self.mc4 = models.MockChroot(

              os_release="fedora", os_version="rawhide", arch="i386", is_active=True)

+         self.mc4.distgit_branch = models.DistGitBranch(name='master')

  

          self.mc_basic_list = [self.mc1, self.mc2, self.mc3, self.mc4]

          # only bind to coprs if the test has used the f_coprs fixture
@@ -191,12 +199,20 @@

                  mc = models.MockChroot(

                      os_release="fedora", os_version=os_version,

                      arch=arch, is_active=True)

+                 # Let's try slashes now. for example.  Some copr instances use

+                 # this pattern.

+                 mc.distgit_branch = BranchesLogic.get_or_create(

+                         'fedora/{0}'.format(os_version),

+                         session=self.db.session)

                  self.mc_list.append(mc)

  

              for os_version in [5, 6, 7]:

                  mc = models.MockChroot(

                      os_release="epel", os_version=os_version,

                      arch=arch, is_active=True)

+                 mc.distgit_branch = BranchesLogic.get_or_create(

+                         'el{0}'.format(os_version),

+                         session=self.db.session)

                  self.mc_list.append(mc)

  

          self.mc_list[-1].is_active = False
@@ -206,7 +222,8 @@

              for mc in self.mc_list:

                  cc = models.CoprChroot()

                  cc.mock_chroot = mc

-                 self.c1.copr_chroots.append(cc)

+                 # TODO: why 'self.c1.copr_chroots.append(cc)' doesn't work here?

+                 cc.copr = self.c1

  

          self.db.session.add_all(self.mc_list)

  

By default copr still follows Fedora branching scheme, though by defining something like:

def my_chroot_to_branch(chroot):
    os, version, _ = chroot.split('-')
    return "{0}/{1}".format(os, version)

DIST_GIT_CHROOT_TO_BRANCH = my_chroot_to_branch                         

You can basically implement the chroot -> branch mapping in configuration file arbitrarily.

rebased

7 years ago

rebased

7 years ago

rebased

7 years ago

rebased

7 years ago

rebased

7 years ago

rebased

7 years ago

Rebased once more, please have a look. I know there's once "looks good" already, but I've added several other changes into this PR.

rebased

7 years ago

rebased

7 years ago

Wouldn't it be better to add support for the new branch naming directly into fedpkg-copr? Like this, you reintroduce usage of concrete branch names in the branch_to_dist, which is something we wanted to get rid of, I thought.

My original thoughts were that we should ideally minimize patching fedpkg-copr, so we could once use the upstream fedpkg tool (or from my POV rather stop using both fedpkg and fedpkg-copr, and use underlying tools only).

Don't we want to merge things from fedpkg-copr into fedpkg?

There is a possibility to use mock --buildsrpm instead of fedpkg-copr srpm for srpm from sources building in backend. For downloading sources from dist-git just fedpkg should do.

I think mock --buildsrpm is too heavy weight (if we gone extreme, rpmbuild would be enough to get source rpm on builder, when sources are downloaded). But anyways ..

I was confused before. What I really dislike about fedpkg-copr is the compatibility burden across fedpkg-copr, fedpkg and pyrpkg. Ideally we should have tool based on pyrpkg only. API-like dependence on fedpkg (the actual status) is bad as fedpkg developers don't care about copr's use-cases (and it wouldn't be fair if we blocked them) thus the "API" is often changed, it maybe isn't API at all.

So the problem is not that fedpkg-copr is wrong place to hack now ... the problem is that we (a) depend on 'fedkpg' and (b) cosmetics: it's name evokes that the tool is related to fedora packaging (fed-pkg), which it is not. Is this correct analysis?

I just filed this, let me know if this doesn't make sense:
https://bugzilla.redhat.com/show_bug.cgi?id=1410403

I think mock --buildsrpm is too heavy weight (if we gone extreme, rpmbuild would be enough to get source rpm on builder, when sources are downloaded). But anyways ..

You also need to have the required macros (distro-related macros used in spec like %dist ) defined when you run rpmbuild, which is why mock --buildsrpm might be good (because they are defined there by buildroot-placed packages with the macro definitions). The idea is that we don't need extra fedpkg-copr to define the macros for rpmbuild in the command line.

This would be about rpmbuild --define 'dist %nil' everywhere, AFAIK, which is what I would do anyways. I don't think this is reason to call mock (which downloads dozens of other packages) just for %dist. Both mock --buildsrpm and raw rpmbuild seem to be extreme to me (compared to the actual status quo).

note that SRPM are distroversion-specific, e.g. %{python3_pkgversion} is 34 on epel7 and 3 on fedora.

@ignatenkobrain can you elaborate how this could break builds of the final SRPM/RPMs from intermediate SRPM?

If that really causes real issues, it sounds like design issue (somewhere) and you should report that. Users need to be able to generate SRCM from dist-git on any distribution (marginal example for us: Debian, but consider RHEL7) -- and then build the RPMs from that SRPM for any distribution/distribution-version (e.g. by mock or by 'copr build PROJECT LOCAL_SRPM).
The only real (and ugly) issue IMO is that EPEL5 chroot is able to only work with digest algorithm md5, but that's bug in epel5 (historically one should make sure other digests are supported on epel5 before we switched a world-default to different digest algorithm for SRPM generators). But IMO nevermind now, we could default to md5 everywhere anyways (till epel5 is supported in copr).

I was requested to add support for RHEL4 chroots (rhbz#1299368) -> TBH, I'm not sure about related caveats WRT SRPM generating, but mock on fedora 25 is not able to build for RHEL4 .. if 'mock --buildsrpm' was helpful there, then I would probably switch my opinion about it.

I made this pull request https://github.com/rpm-software-management/mock/pull/41 to finally move this issue forward. It is an optimization upon the mock --buildsrpm idea, using mock-scm plugin. We will see if it is accepted.

Well, at the time being ... I am worried about dist-git machine performance. Even now internal copr faces massive dist-git performance issues when somebody schedules tito/mock-scm build, and it looks this is going to make it even worse. Mock IMO brings zero benefit here and initializing the caches costs a lot. Why not simply do:

rpkg clone
rpkg srpm

(as described in https://bugzilla.redhat.com/show_bug.cgi?id=1410403)?

That sounds like trivial and the cheapest way (zero overhead). Is PoC against copr sources needed, too? I panned to not work on this more, unless I have a pre-ack (no obvious issue).

Well, at the time being ... I am worried about dist-git machine performance. Even now internal copr faces massive dist-git performance issues when somebody schedules tito/mock-scm build, and it looks this is going to make it even worse.

This should run on copr-backend machine and mock-scm does only the two required things with the remote dist-git: checking out sources and fetching tarball so there is no increased load on copr-dist-git machine.

Mock IMO brings zero benefit here and initializing the caches costs a lot.

You can build rpm directly with mock-scm so cache gets initialized only once.

Why not simply do:

rpkg clone
rpkg srpm

The updated plugin does rpkg clone and rpkg sources.

(as described in https://bugzilla.redhat.com/show_bug.cgi?id=1410403)?
That sounds like trivial and the cheapest way (zero overhead).

There is no overhead in terms of performance but there is a programming overhead in the macro definition maintanance for various distros. You need to assure that the macros are defined either by the system or as command-line arguments for rpmbuild. By building rpms already in the target arch, you get that for free. Also this brings a potential new features when user can add into the chroot his own macros by an "additonal package" added to root-cache.

Is PoC against copr sources needed, too? I panned to not work on this more, unless I have a pre-ack (no obvious issue).

I appreciate your work there but the script adds unnecessary maintenance.

This should run on copr-backend machine

Ah, truth. From the long term POV though it should be builder-controlled.

Would this be compatible with "slash" type of branching now, right?

You can build rpm directly with mock-scm so cache gets initialized only once.

Ok. I expect this will break custom-* mock configs, because the minimal buildroot will not be that minimal one would expect, just because of generating srpm first. Maybe I miss something..

There is no overhead in terms of performance but there is a programming overhead in the macro definition maintanance for various distros.

When we talk about srpm generator on builder before submitted to mock, there shouldn't be such macros. So this is unnecessary worry.

Also, from "long-term" perspective we'll need copr's dist-git client anyway, right? So people could:
1. push to dist-git (by client)
2. upload sources to lookaside cache
3. build against particular chroot (the same as regular fedora packaging works)
Instead of:
1. keeping packages in separate dist-git
2. generate srpm to be sent to copr
3. dist-git to download the srpm
4. dist-git to decouple the srpm and push it to git/lookaside cache

For this to work, there will be alternative to fedpkg for copr (both should be only configuration packages for rpkg).

Note that the dist-git-client side would be pretty trivial, once we have resolved user-authentication for dist-git pushes...

When we talk about srpm generator on builder before submitted to mock, there shouldn't be such macros. So this is unnecessary worry.

Oh, there are. Please have a look at definitions in fedpkg:init:load_rpmdefines and then at copr/dist-git-client/fedpkg-client:load_rpmdefines.

Also, from "long-term" perspective we'll need copr's dist-git client anyway, right? So people could:
1. push to dist-git (by client)
2. upload sources to lookaside cache
3. build against particular chroot (the same as regular fedora packaging works)
Instead of:
1. keeping packages in separate dist-git
2. generate srpm to be sent to copr
3. dist-git to download the srpm
4. dist-git to decouple the srpm and push it to git/lookaside cache
For this to work, there will be alternative to fedpkg for copr (both should be only configuration packages for rpkg).

copr-dist-git, as a part of our pipe-line, is used to gather different source inputs and make dist-git repositories out of them, which are then used as inputs for rpm builds on backend. Also, it serves as back-log for build history. For these two functions, there is no need to make another copr-specific rpkg-based tool. Also note that the second workflow is simplified by the dist-git build feature, that will be hopefully extended to more than just Fedora DistGit.

I'm aware of those, but those are completely useless for us in this case. We need working
srpm, and mock generates the right srpm for us anyway (part of build results).

I mean -- you generate src.rpm by mock just to submit that src.rpm to mock which
is going to generate the src.rpm again ...

We don't need mock in the first step, and the macros neither.

Pagure is obviously not my friend ...

Trying to get the previous reply again:

Oh, there are. Please have a look at definitions in
fedpkg:init:load_rpmdefines and then at
copr/dist-git-client/fedpkg-client:load_rpmdefines.

I'm aware of those, but we don't need them, as is mentioned in
rhbz#1410403.

I mean -- what you propose is -- you generate src.rpm by mock just to
submit that src.rpm to mock again which in turn is going to generate the
src.rpm (with correct macros) again. So no need to bother with those
macros in the first step.

copr-dist-git, as a part of our pipe-line, is used to ...

What you describe is the current status, but there are RFEs to move
forward (and actually those would be the most useful RFEs so far).
Not that hard to implement I guess, the worst part is authentication..

Pagure is obviously not my friend ...
Trying to get the previous reply again:

Oh, there are. Please have a look at definitions in
fedpkg:init:load_rpmdefines and then at
copr/dist-git-client/fedpkg-client:load_rpmdefines.

I'm aware of those, but we don't need them, as is mentioned in
rhbz#1410403.

Could you point me to a specific place where this is mentioned? I am quite sure we need them as you cannot build a srpm with incomplete set of macro defintions for a given spec file.

I mean -- what you propose is -- you generate src.rpm by mock just to
submit that src.rpm to mock again which in turn is going to generate the
src.rpm (with correct macros) again. So no need to bother with those
macros in the first step.

I think you miss my point. You can build rpm directly with the mock-scm plugin while generating srpm at the same time. So, currently with fedpkg-copr, it actually is the case that srpm is generated by fedpkg-copr and then again re-generated by mock. But it certainly is not the case in what I propose.

copr-dist-git, as a part of our pipe-line, is used to ...

What you describe is the current status, but there are RFEs to move
forward (and actually those would be the most useful RFEs so far).
Not that hard to implement I guess, the worst part is authentication..

Well, I described a role of copr-dist-git in COPR stack.

Could you point me to a specific place where this is mentioned?

It is more like result from the discussion .. nobody told so far what are those actually needed for ... mock re-generates it's own srpm anyway (from completely unchanged spec file, with correct macros) so I'm curious what we are afraid of.

I think you miss my point. You can build rpm directly with the mock-scm

Cool, I'm convinced now this is promising then, thanks. Will this work with systemd-nspawn (with disabled network)? What happens with custom chroots in general .. (afaik there's no specified minimal set of packages).

Well, I described a role of copr-dist-git in COPR stack.

I was not discussing the original design decisions, but I would be curious whether this is really the "final" mission of copr-dist-git or rather intermediate step. I just remember that I was told by previous devel that we should be able to work with copr from bottom->up (git/cache -> bulid) (instead of the other way around (build srpm -> submit to git), there's rhbz#1397508 but I've seen other too.

Could you point me to a specific place where this is mentioned?

It is more like result from the discussion .. nobody told so far what are those actually needed for ... mock re-generates it's own srpm anyway (from completely unchanged spec file, with correct macros) so I'm curious what we are afraid of.

Well, I told you. You need to know all the macros in the spec to make srpm from it and the sources with rpmbuild.

I think you miss my point. You can build rpm directly with the mock-scm

Cool, I'm convinced now this is promising then, thanks. Will this work with systemd-nspawn (with disabled network)? What happens with custom chroots in general .. (afaik there's no specified minimal set of packages).

I don't see any problem with custom chroots. Could you elaborate more on this?

Well, I described a role of copr-dist-git in COPR stack.

I was not discussing the original design decisions, but I would be curious whether this is really the "final" mission of copr-dist-git or rather intermediate step. I just remember that I was told by previous devel that we should be able to work with copr from bottom->up (git/cache -> bulid) (instead of the other way around (build srpm -> submit to git), there's rhbz#1397508 but I've seen other too.

I don't know the future so I can't really comment on this. But copr-dist-git, as it is now, cannot be very easily mixed with direct user access. That's just because it is auto-updated from the acquired source rpms. With it, you can inspect what you have actually built on patch/spec level.

Well, I told you. You need to know all the macros in the spec to make srpm from it and the sources with rpmbuild.

Can you give me an example of spec file where the macros are needed to generate the src.rpm? That would be almost certainly bug in spec file, ... but haven't seen such buggy spec file so far.

don't see any problem with custom chroots. Could you elaborate more on this?

The problem is that build of SRPM could require more packages in mock chroot than the build of binary RPM.

copr-dist-git, as it is now, cannot be very easily mixed with direct user access.

It is not that easy, because you need to resolve the access control lists somehow. But otherwise it is not really in collision with other build methods...

Well, I told you. You need to know all the macros in the spec to make srpm from it and the sources with rpmbuild.

Can you give me an example of spec file where the macros are needed to generate the src.rpm? That would be almost certainly bug in spec file, ... but haven't seen such buggy spec file so far.

All spec files have that property. That's because rpmbuild parses the spec files during srpm generation and it needs all the macros defined for it.

don't see any problem with custom chroots. Could you elaborate more on this?

The problem is that build of SRPM could require more packages in mock chroot than the build of binary RPM.

Not sure about this and I don't see a problem even if that was so.

copr-dist-git, as it is now, cannot be very easily mixed with direct user access.

It is not that easy, because you need to resolve the access control lists somehow. But otherwise it is not really in collision with other build methods...

Note that you can use rpkg compatible tool to get a repo and sources from copr-dist-git, then make changes, commit, build a srpm locally (with that same tool) and submit that srpm by using copr-cli into COPR, which will get the changes back into copr-dist-git while also building the package. You can hardly improve on that.

I think It would perhaps be good to continue this conversation somewhere more suited for it.

Can you give me an example of spec file where the macros are needed to generate the src.rpm? That would be almost certainly bug in spec file, ... but haven't seen such buggy spec file so far.

All spec files have that property. That's because rpmbuild parses the spec files during srpm generation and it needs all the macros defined for it.

Nope, that's not truth. The macros we talk about (done by fedpkg's load_rpmdefines) are %rhel, %fedora, %dist right? Those only affect the resulting src.rpm name -> your-package-version.fc26.src.rpm on rawhide, your-package-version.el7.src.rpm on RHEL7). We talk about the intermediate src.rpm (not the final one to be stored in results), right? Which means that we don't care about naming at all.

So again, I'd like to see the problematic spec file.

The problem is that build of SRPM could require more packages in mock chroot than the build of binary RPM.
Not sure about this and I don't see a problem even if that was so.

I thought that the custom chroots are right about that -- minimizing the minimal buildroot pollution (for many reasons like distro minimization or bootstrapping).

I think It would perhaps be good to continue this conversation somewhere more suited for it.

Agreed :)

rebased

6 years ago

Ah, there's one lost comment here (have you, Pagure, lost it?).

I've updated the pull request so now there's either Fedora branching scheme, or arbitrary scheme defined by copr frontend configuration. Please take a look.

rebased

6 years ago

rebased

6 years ago

Ah, there's one lost comment here (have you, Pagure, lost it?).
I've updated the pull request so now there's either Fedora branching scheme, or arbitrary scheme defined by copr frontend configuration. Please take a look.

Note that what is probably needed here is really just a "frontend configuration", that is mapping in config like:

fedora-25 = f25
fedora-26 = f26
fedora-rawhide = master
epel-6 = el6
mageia-6 = mga6

etc.

Agreed, this mapping is there by default, and can be changed by configuration. For some years we use mapping 'DISTRO-VERSION-ARCH' -> 'DISTRO/VERSION'. So the point is to allow me (or anybody else) to change the mapping.

More on this -> from now on you can add specific mapping without patching the copr code; e.g. you could simply do ./manage.py create_chroot mageia-couldron-x86_64 -b couldron.

Note that the mapping is needed both ways, chroot -> branch and branch -> chroots; previously we maintained two python "convert" methods -- now the mapping is more clear and deterministic (given by db relations); probably no more code change from this POV will ever be needed.

rebased

6 years ago

Note that the mapping is needed both ways, chroot -> branch and branch -> chroots; previously we maintained two python "convert" methods -- now the mapping is more clear and deterministic (given by db relations); probably no more code change from this POV will ever be needed.

Yes, but it's much more maintainable for the mapping to be placed just in the copr-frontend config file:

Then:

1) it can be easily changed anytime
2) you don't need to change db scheme
3) you don't need any other "custom" code for the mapping generation

Just, please, use the config file itself to define the mapping (with a newly introduced ini section or maybe you can also just do something like DISTGIT_MAPPING = {"fedora-25" : "f25", "fedora-26" : "f26", "epel6" : "el6" }).

1) it can be easily changed anytime

I don't want to have this in config file, because that might make user think
that changing the layout is "as easy as changing the configuration" ... that's
simply not truth -> because by changing the config you de-sync 'dist-git' data
with 'frontend' config. It makes sense to track the "hard-wired" data in
database so changing the old mapping is rather "refused".

Also note the mapping is supposed to be 1:N, not 1:1.

2) you don't need to change db scheme

But the migration was already written, so yeah - with believe that I do a good thing, I already wasted the time with this :)

3) you don't need any other "custom" code for the mapping generation

From this POV the actual PR is actually equivalently good :/

--

Please, before we refuse this (I spent quite some time on that already) please reconsider
whether you can not bend the rules. Unless that's significant problem, I would appreciate that (pagure is hell slow again ...).

Note that this PR allowed me to drop the ugly branch_to_os_version() and we could also drop the chroot_to_branch perhaps ...

rebased

6 years ago

1) it can be easily changed anytime

I don't want to have this in config file, because that might make user think
that changing the layout is "as easy as changing the configuration" ... that's
simply not truth -> because by changing the config you de-sync 'dist-git' data
with 'frontend' config. It makes sense to track the "hard-wired" data in
database so changing the old mapping is rather "refused".
Also note the mapping is supposed to be 1:N, not 1:1.

2) you don't need to change db scheme

But the migration was already written, so yeah - with believe that I do a good thing, I already wasted the time with this :)

3) you don't need any other "custom" code for the mapping generation

From this POV the actual PR is actually equivalently good :/

Please, before we refuse this (I spent quite some time on that already) please reconsider
whether you can not bend the rules. Unless that's significant problem, I would appreciate that (pagure is hell slow again ...).

Sorry, but that DIST_GIT_CHROOT_TO_BRANCH construction where you can put a method ("custom code") into a configuration option is not acceptable. Btw. the suggested config mapping is also 1:n, of course. Complex solutions can bring lots of maintenance issues so I think it's worth taking another shot.

I think it's worth taking another shot.

Not for me ATM. Working on another PR atm.

I don't understand what's wrong on DIST_GIT_CHROOT_TO_BRANCH, so I'll keep this open another year or so and return back once the downstream patching kicks me again ... :)

One attempt more this time :)

Complex solutions can bring lots of maintenance

Define what's complex please ... I almost removed more code than I added (not counting verbose comments and migration script, which is not really "the maintenance issue").

Sorry, but that DIST_GIT_CHROOT_TO_BRANCH construction where you can put a method ("custom code") into a configuration option is not acceptable.

Can you say why? The config option is admin writeable, so it is up to him how he is going to break his app. Also, if that's an issue -- can I consider implementing this "hook" as "plugin" elsewhere then in config?

Or what if I dropped that DIST_GIT_CHROOT_TO_BRANCH convenience thingy entirely? I can live with ./manage.py create_chroot --distgit-branch option during chroot creating ...

rebased

6 years ago

Can you please have another look? Is the first patch at least acceptable upstream? I can downstream patch the second one temporarily for the migration time, and then depend on ./manage.py --dist-git-branch explicit option.

Just the first patch (without the second) is generally acceptable.

1 new commit added

  • [frontend][dist-git][backend] arbitrary dist-git branching
6 years ago

Pull-Request has been closed by praiskup

6 years ago

I pushed from command-line, because pagure strikes now. Thanks again.

Pull-Request has been merged by praiskup

6 years ago

Pull-Request has been merged by praiskup

6 years ago

Pull-Request has been merged by praiskup

6 years ago

Note that I wanted to give it one more review before merging (after you would have actually removed the second patch).

Ah, that was misunderstanding .. can we solve review notes via additional issue then?