Bug 1410403

Summary: fedpkg-copr: don't depend on fedpkg
Product: [Community] Copr Reporter: Pavel Raiskup <praiskup>
Component: backendAssignee: clime
Status: CLOSED CURRENTRELEASE QA Contact:
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: unspecifiedCC: clime, praiskup
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-08-03 13:08:15 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Pavel Raiskup 2017-01-05 12:23:06 UTC
Fedpkg changes often, which is not a bug;  fedpkg is just client tool not
providing API.  That's strictly related to fedora packaging.

That's why `fedpkg-copr` package should not depend on `fedpkg`.  Both
`dist-git` and `builders` should only depend on API in `pyrpkg`.  That
should result in less headaches with copr build-system upgrades.

Also, if we were able to switch out from fedpkg, we can easily differ from
Fedora specific guidelines (e.g. dist-git branch names).

Comment 1 Pavel Raiskup 2017-01-05 12:26:09 UTC
(In reply to Pavel Raiskup from comment #0)
> we can easily differ from Fedora specific guidelines (e.g. dist-git branch
> names).

Read that rather like Fedora/Koji-specific build-system _configuration_,
this has nothing to do with Fedora Packaging Guidelines.

Comment 2 clime 2017-01-11 11:12:06 UTC
Well, dropping fedpkg means dropping load_rpmdefines functionality that we inherit in fedpkg-copr - that means dropping support for Fedora dist-git branch naming. For COPR deployments that currently use Fedora branch naming (like Fedora COPR), that would be a problem.

Comment 3 Pavel Raiskup 2017-01-11 11:47:13 UTC
(In reply to clime from comment #2)
> Well, dropping fedpkg means dropping load_rpmdefines functionality that we
> inherit in fedpkg-copr

load_rpmdefines() is also implemented in pyrpkg, and sounds like there's only
one (pretty small) difference in it's output:


| -                    "--define 'dist .%s'" % self._disttag,
| +                    "--define 'dist .%s'" % self.dist,
|                      "--define '%s %s'" % (self._distvar,
| -                                          self._distval.split('_')[0]),
| -                    # int and float this to remove the decimal
| -                    "--define '%s 1'" % self._disttag]
| +                                          self._distval),
| +                    "--eval '%%undefine %s'" % self._distunset,
| +                    "--define '%s 1'" % self.dist]

The 'self.dist' is trivial to compute, I think in Copr we already re-calculate
this stuff at least for mageia.

(In reply to clime from comment #2)
> For COPR deployments that currently use Fedora branch naming (like
> Fedora COPR), that would be a problem.

I think it is useful for Fedora Copr (such deployment) to not depend on
fedpkg with this regard;  so such deployments would benefit from
re-implementing the "branch to dist" mapping, so it is immutable by fedpkg
folks.

Does that make sense ^^, so should I assign this bug to myself and propose
PR (possibly fix the  https://pagure.io/copr/copr/pull-request/11)?

Comment 4 clime 2017-01-11 13:41:27 UTC
(In reply to Pavel Raiskup from comment #3)
> (In reply to clime from comment #2)
> > Well, dropping fedpkg means dropping load_rpmdefines functionality that we
> > inherit in fedpkg-copr
> 
> load_rpmdefines() is also implemented in pyrpkg, and sounds like there's only
> one (pretty small) difference in it's output:

load_rpmdefines in fedpkg is pretty much completely reimplemented (extended a lot if compared to pyrpkg) containing definitions for various Fedora dist-git branches such as 'f\d\d$' or 'el\d$'. This is what we need currently for fedpkg-copr to operate correctly in copr-backend when Fedora dist-git naming is used.

>I think it is useful for Fedora Copr (such deployment) to not depend on
>fedpkg with this regard;  so such deployments would benefit from
>re-implementing the "branch to dist" mapping, so it is immutable by fedpkg
>folks.

I am not getting this. Do you plan to reimplement what is in fedpkg into our dist-git-client (called fedpkg-copr currently)?

Comment 5 Pavel Raiskup 2017-01-11 14:12:53 UTC
(In reply to clime from comment #4)
> load_rpmdefines in fedpkg is pretty much completely reimplemented
> (extended a lot if compared to pyrpkg) containing definitions for
> various Fedora dist-git branches such as 'f\d\d$' or 'el\d$'. This is
> what we need currently for fedpkg-copr to operate correctly in
> copr-backend when Fedora dist-git naming is used.

But just for "branch -> dist" mapping (we don't need anything else!), the
dependency on fedpkg is IMO too much (taking into account that we reimplement
the mapping anyway, even though "only" temporarily).

The branching policy in "general" Copr deployment is different topic from
branching in Fedora.

> I am not getting this. Do you plan to reimplement what is in fedpkg into
> our dist-git-client (called fedpkg-copr currently)?

I think some light-weight version of it, yes.  To have only that part which we
necessarily need and drop the 'fedpkg' requirement.

Comment 6 Pavel Raiskup 2017-01-11 14:18:55 UTC
Maybe, if you consider dependency on 'fedpkg' important -- we could inherit
from 'pyrpkg' classes (and inherit from 'fedpkg' classes only optionally).

I believe that 'fedpkg' is *not* desired to be library at all, btw.  There's
actually only one version of fedpkg (no python2 and python3 version), which
might look like there's need for completely different pythonX-fedpkg library
providing "only" the actual fedoras "branch -> dist" mapping.  But still,
this should be optional for Copr.

Comment 7 Pavel Raiskup 2017-01-11 14:25:09 UTC
Ah, pyrpkg is python2 only, too.  But that's supposed to be library.

Comment 8 clime 2017-01-11 14:28:20 UTC
(In reply to Pavel Raiskup from comment #5)
I am not getting this. Do you plan to reimplement what is in fedpkg into
> > our dist-git-client (called fedpkg-copr currently)?
> 
> I think some light-weight version of it, yes.  To have only that part which
> we
> necessarily need and drop the 'fedpkg' requirement.

I don't really see a reason for dropping fedpkg. I do not consider its API to be that unstable to necessitate a change.

Comment 9 Pavel Raiskup 2017-01-11 14:35:11 UTC
That's not really API though, and it brings just constraints (zero
additional functionality from Copr POV, no flexibility), e.g. 'master' ==
rawhide.

Without pushing you somewhere you don't want to be, is it OK to make
fedpkg "optional" dependency (installed by ansible instead of Requires)?

Comment 10 Pavel Raiskup 2017-01-11 14:37:45 UTC
Also note that 'mga' proposal in
https://pagure.io/fedpkg/pull-request/25
goes totally out of scope fedpkg -- I would be really surprised if there was
small motivation for merge.  Fedpkg is really Fedora oriented, I would expect
something like 'mgapkg' tool.

Comment 11 clime 2017-01-11 14:54:01 UTC
(In reply to Pavel Raiskup from comment #9)
> That's not really API though, and it brings just constraints (zero
> additional functionality from Copr POV, no flexibility), e.g. 'master' ==
> rawhide.
> 
> Without pushing you somewhere you don't want to be, is it OK to make
> fedpkg "optional" dependency (installed by ansible instead of Requires)?

I don't get the point of the change. We need functionality that we inherit from fedpkg because of the support for Fedora named branches and to duplicate code that is already there seems not to be great.

Comment 12 Pavel Raiskup 2017-01-11 15:19:01 UTC
(In reply to clime from comment #11)
> I don't get the point of the change.  We need functionality that we
> inherit from fedpkg because of the support for Fedora named branches

That functionality is needed solely for one instance Fedora Copr (and I
bet even there it is temporary anyways, with the huge
containerization/taskotron/modularity efforts I would be surprised if the
branching scheme survived forever).

> and to duplicate code that is already there seems not to be great.

Ok, but we duplicate it anyways (temporarily) but the PR is there for a
long time already..
---

Comment 13 Pavel Raiskup 2017-01-11 15:20:46 UTC
Argh, I hit "submit" too early .. I agree that duplicating the functionality
is a bad idea.  But as no other Copr instance will benefit from 'fedpkg'
dependency, could I hack on making it optional?

Comment 14 clime 2017-01-12 14:26:35 UTC
(In reply to Pavel Raiskup from comment #13)
> Argh, I hit "submit" too early .. I agree that duplicating the functionality
> is a bad idea.  But as no other Copr instance will benefit from 'fedpkg'
> dependency, could I hack on making it optional?

Do as you like. I think much better direction is to try to employ `mock --buildsrpm` and get rid of the whole fedpkg-copr, fedpkg, pyrpkg stack at once while keeping support for Fedora named branches. This is probably what I will look at, if noone else will be faster, because I find it very tempting to build srpm in that same chroot where the rpm is built, instead of building it outside of the chroot and then passing it into the chroot for rpm-building as is the case now.

Even if that didn't work, removing fedpkg dependency from fedpkg-copr package is not bringing anything and I would suggest to make a new script aside as a proof of concept that in some way (re-)implements fedpkg-copr's functionality as it stands now and is used by copr-backend to obtain srpms from copr-dist-git.

Comment 15 Pavel Raiskup 2017-01-12 15:33:24 UTC
(In reply to clime from comment #14)
> I think much better direction is to try to employ `mock --buildsrpm` and
> get rid of the whole fedpkg-copr, fedpkg, pyrpkg stack at once while
> keeping support for Fedora named branches.

At least 'pyrpkg' is still needed to clone the package and download
sources.

> This is probably what I will look at, if noone else will be faster,
> because I find it very tempting to build srpm in that same chroot where
> the rpm is built, instead of building it outside of the chroot and then
> passing it into the chroot for rpm-building as is the case now.

Note that "minimal" buildroot might be modified (explicitly added packages
by copr maintainer.

Also is "clean" needed between mock "--buildsrpm" and "--rebuild"?  If
yes, it will take additional time to generate source rpm, and if not, one
should ensure that the mock chroot is not modified some ugly way so
--rebuild goes to FTBFS.

I pretty much like your idea about 'mock --buildsrpm' from long term
perspective -- we should generate one SRPM for e.g. Fedora 25 and then
build this SRPM against all Fedora 25 architectures enabled in copr
(aka koji/brew style).

> Even if that didn't work, removing fedpkg dependency from fedpkg-copr
> package is not bringing anything and I would suggest to make a new
> script aside as a proof of concept that in some way (re-)implements
> fedpkg-copr's functionality as it stands now and is used by copr-backend
> to obtain srpms from copr-dist-git.

Good idea.  Let's call that script 'copkg'.

Comment 16 Pavel Raiskup 2017-01-13 12:23:53 UTC
PoC:
https://github.com/praiskup/copr-dist-git-client-poc

You can try e.g. this against Fedora copr:
./prep-srpm --config config-copr.sh --module praiskup/ping/quick-package --revision=5ce6ec838e22df0c5dfbb3ca042347a04829f5c1

Comment 18 Pavel Raiskup 2017-01-13 12:25:45 UTC
Note that I'm not sure about %dist and %fedora/%rhel/.. macros.  If there
was usage for them, it is trivial to implement the mapping according to
our needs.

Comment 19 Pavel Raiskup 2017-03-20 06:32:31 UTC
(In reply to clime from comment #14)
> I would suggest to make a new script aside as a proof of concept that in some
> way (re-)implements fedpkg-copr's functionality as it stands now and is used
> by copr-backend to obtain srpms from copr-dist-git.

Here is another attempt, btw. using 'mock --buildsrpm' as suggested in
previous discussion [1].  I kind of dislike this fact, but as long as we
support mageia (and they did some unfortunate decisions complicating
generating of source RPMs on non-mageia boxes [2] -- I guess that's distro
bug), the 'mock --buildsrpm' is needed (and used ATM in the concept).

There's one neat thing about this copr-builder script:  When we use it
together with [3] pull request, we have live build logs almost for free :).
That's because with builder-side script it is much easier to work with mock to
produce readable logs.

There are some incompatibility changes like moving from 'mockchain' to 'mock'
(I'm still not sure whether this is needed) and using 'copr mock-config'
feature to get the mock configuration (so at least [4] is needed too).

I'll cover pros/cons in upcoming pull-request (once done) against copr-builder
which should (at least optionally) utilize this script.

[1] https://github.com/praiskup/copr-bulider
[2] https://pagure.io/copr/copr/c/13b53c9d6706
[3] https://pagure.io/copr/copr/pull-request/44

Comment 20 Pavel Raiskup 2017-03-20 06:33:59 UTC
[4] https://pagure.io/copr/copr/pull-request/48

Comment 21 clime 2017-08-03 13:08:15 UTC
fedpkg-copr is no longer used in COPR and has been deprecated as a package.