[PATCH] Allow spec file to be in a different path than /

[Thomas]

This should allow people to build from git repositories that have a
different structure than Fedora (they still need some magic with
source_cmd). The patch is not too intrusive and can be set per
repository.

Let me know if it can be considered for inclusion. I am happy to rework it.

---
diff --git a/builder/kojid b/builder/kojid
index b3d0d1c..89f9474 100755
--- a/builder/kojid
+++ b/builder/kojid
@@ -3292,7 +3292,7 @@ class BuildSRPMFromSCMTask(BaseBuildTask):
         self.patch_scm_source(sourcedir, logfile, opts)

         # Find and verify that there is only one spec file.
-        spec_files = glob.glob("%s/*.spec" % sourcedir)
+        spec_files = glob.glob("%s/%s*.spec" % (sourcedir, scm.spec_path))
         if len(spec_files) == 0:
             raise koji.BuildError("No spec file found")
         elif len(spec_files) > 1:
diff --git a/koji/daemon.py b/koji/daemon.py
index 205f62e..68af212 100644
--- a/koji/daemon.py
+++ b/koji/daemon.py
@@ -200,6 +200,7 @@ class SCM(object):
         - use_common (defaults to True, may be set by assert_allowed())
         - source_cmd (defaults to ['make', 'sources'], may be set by
assert_allowed())
         - scmtype
+        - spec_path

         The exact format of each attribute is SCM-specific, but the
structure of the url
         must conform to the template above, or an error will be raised.
@@ -220,6 +221,7 @@ class SCM(object):
         self.revision = fragment
         self.use_common = True
         self.source_cmd = ['make', 'sources']
+        self.spec_path = '/'

         for scmtype, schemes in SCM.types.items():
             if self.scheme in schemes:
@@ -311,6 +313,10 @@ class SCM(object):
                         else:
                             # there was nothing after the trailing :,
so they don't want to run a source_cmd at all
                             self.source_cmd = None
+                    # check if spec path is defined
+                    if len(scm_tuple) >= 5:
+                        if scm_tuple[4]:
+                            self.spec_path = scm_tuple[4] + '/'
                     break
             else:
                 self.logger.warn('Ignoring incorrectly formatted SCM
host:repository: %s' % allowed_scm)

[Mathieu Bridon]

Disclaimer: I'm not a Koji maintainer

On Wed, 2014-08-06 at 17:42 +0200, Thomas wrote:
>  +                    # check if spec path is defined
>  +                    if len(scm_tuple) >= 5:
>  +                        if scm_tuple[4]:
>  +                            self.spec_path = scm_tuple[4] + '/' 
Not commenting on whether this is desired or acceptable, but if you're
adding a new part to the allowed_scms tuple, could you also document it
in the kojid.conf file, to help admins know about this parameter?

Right now there already is an undocumented option (source_cmd) which
admins can only discover by reading the source code. I just sent a patch
to fix that, but it would be nice if we didn't add one more undocumented
option. :)


-- 
Mathieu

[Thomas]

Good point I'll add your patch and update the kojid.conf. thanks.

On Thu, Aug 7, 2014 at 10:01 AM, Mathieu Bridon
<bochecha(a)fedoraproject.org&gt; wrote:
>  Disclaimer: I'm not a Koji maintainer
>
>  On Wed, 2014-08-06 at 17:42 +0200, Thomas wrote:
> > +                    # check if spec path is defined
> > +                    if len(scm_tuple) >= 5:
> > +                        if scm_tuple[4]:
> > +                            self.spec_path = scm_tuple[4] + '/'
>
>  Not commenting on whether this is desired or acceptable, but if you're
>  adding a new part to the allowed_scms tuple, could you also document it
>  in the kojid.conf file, to help admins know about this parameter?
>
>  Right now there already is an undocumented option (source_cmd) which
>  admins can only discover by reading the source code. I just sent a patch
>  to fix that, but it would be nice if we didn't add one more undocumented
>  option. :)
>
>
>  --
>  Mathieu
>
>  --
>  buildsys mailing list
>  buildsys(a)lists.fedoraproject.org
>  https://admin.fedoraproject.org/mailman/listinfo/buildsys 


-- 
Thomas

[Mike McLean]

On 08/06/2014 11:42 AM, Thomas wrote:
>  This should allow people to build from git repositories that have a
>  different structure than Fedora (they still need some magic with
>  source_cmd). The patch is not too intrusive and can be set per
>  repository. 
I really hate to add yet another flag to the already tedious scm config
format, especially when I really doubt we'll have cause to set it to
anything other than these two values. Probably better just to do
something like this:

@@ -3417,6 +3417,9 @@ class BuildSRPMFromSCMTask(BaseBuildTask):

         # Find and verify that there is only one spec file.
         spec_files = glob.glob("%s/*.spec" % sourcedir)
+        if not spec_files:
+            # also check SPECS dir
+            spec_files = glob.glob("%s/SPECS/*.spec" % sourcedir)
         if len(spec_files) == 0:
             raise koji.BuildError("No spec file found")
         elif len(spec_files) > 1:

This much gets you halfway there, but it looks like the system is just
going to choke later on having the sources under SOURCES instead of the
git root dir.

I guess you could set up a source_cmd to move or link the sources
around, but that's hardly a clean solution.

I wouldn't mind teaching koji to handle this layout, but it looks like
it might be a little more involved than just the spec portion (unless
I'm missing something). Did you have a solution for the SOURCES dir?

>  Let me know if it can be considered for inclusion. I am happy to
> rework it.
>  
>  ---
>  diff --git a/builder/kojid b/builder/kojid
>  index b3d0d1c..89f9474 100755
>  --- a/builder/kojid
>  +++ b/builder/kojid
>  @@ -3292,7 +3292,7 @@ class BuildSRPMFromSCMTask(BaseBuildTask):
>           self.patch_scm_source(sourcedir, logfile, opts)
>  
>           # Find and verify that there is only one spec file.
>  -        spec_files = glob.glob("%s/*.spec" % sourcedir)
>  +        spec_files = glob.glob("%s/%s*.spec" % (sourcedir, scm.spec_path))
>           if len(spec_files) == 0:
>               raise koji.BuildError("No spec file found")
>           elif len(spec_files) > 1:
>  diff --git a/koji/daemon.py b/koji/daemon.py
>  index 205f62e..68af212 100644
>  --- a/koji/daemon.py
>  +++ b/koji/daemon.py
>  @@ -200,6 +200,7 @@ class SCM(object):
>           - use_common (defaults to True, may be set by assert_allowed())
>           - source_cmd (defaults to ['make', 'sources'], may be set by
>  assert_allowed())
>           - scmtype
>  +        - spec_path
>  
>           The exact format of each attribute is SCM-specific, but the
>  structure of the url
>           must conform to the template above, or an error will be raised.
>  @@ -220,6 +221,7 @@ class SCM(object):
>           self.revision = fragment
>           self.use_common = True
>           self.source_cmd = ['make', 'sources']
>  +        self.spec_path = '/'
>  
>           for scmtype, schemes in SCM.types.items():
>               if self.scheme in schemes:
>  @@ -311,6 +313,10 @@ class SCM(object):
>                           else:
>                               # there was nothing after the trailing :,
>  so they don't want to run a source_cmd at all
>                               self.source_cmd = None
>  +                    # check if spec path is defined
>  +                    if len(scm_tuple) >= 5:
>  +                        if scm_tuple[4]:
>  +                            self.spec_path = scm_tuple[4] + '/'
>                       break
>               else:
>                   self.logger.warn('Ignoring incorrectly formatted SCM
>  host:repository: %s' % allowed_scm)
>  --
>  buildsys mailing list
>  buildsys(a)lists.fedoraproject.org
>  https://admin.fedoraproject.org/mailman/listinfo/buildsys
>

[Mike McLean]

On 08/13/2014 06:05 PM, Mike McLean wrote:
>  This much gets you halfway there, but it looks like the system is
> just
>  going to choke later on having the sources under SOURCES instead of the
>  git root dir.
>  
>  I guess you could set up a source_cmd to move or link the sources
>  around, but that's hardly a clean solution.
>  
>  I wouldn't mind teaching koji to handle this layout, but it looks like
>  it might be a little more involved than just the spec portion (unless
>  I'm missing something). Did you have a solution for the SOURCES dir? 
Actually not that complicated after all. I did add an option to turn the
support off in case it causes a problem (possible that someone could
have a SOURCES dir in their checkout for some other reason).

Anyway, here is where I'm at. Seems to be working.

[Thomas]

Hi All,

I updated your patch so it does not assume sourcedir=chrootdir
But it's not yet the perfect solution.

For reference the layout is:
SPEC/
SOURCES/
.metadata-file

diff --git a/builder/kojid b/builder/kojid
index 821f19d..4210823 100755
--- a/builder/kojid
+++ b/builder/kojid
@@ -455,6 +455,10 @@ class BuildRoot(object):

     def build_srpm(self, specfile, sourcedir, source_cmd):
         self.session.host.setBuildRootState(self.id,'BUILDING')
+        if self.options.support_rpm_source_layout:
+            sources_dir = "%s/SOURCES" % sourcedir
+       else:
+           sources_dir = sourcedir
         if source_cmd:
             # call the command defined by source_cmd in the chroot so
any required files not stored in
             # the SCM can be retrieved
@@ -466,7 +470,7 @@ class BuildRoot(object):
                 self.expire()
                 raise koji.BuildError, "error retrieving sources, %s"
% self._mockResult(rv)

-        args = ['--no-clean', '--buildsrpm', '--spec', specfile,
'--sources', sourcedir,
+        args = ['--no-clean', '--buildsrpm', '--spec', specfile,
'--sources', sources_dir,
                 '--target', 'noarch']

         rv = self.mock(args)
@@ -3793,10 +3797,7 @@ class BuildSRPMFromSCMTask(BaseBuildTask):

         #build srpm
         self.logger.debug("Running srpm build")
-        sources_dir = "%s/SOURCES" % sourcedir
-        if not self.options.support_rpm_source_layout or not
os.path.isdir(sources_dir):
-            sources_dir = sourcedir
-        broot.build_srpm(spec_file, sources_dir, scm.source_cmd)
+        broot.build_srpm(spec_file, sourcedir, scm.source_cmd)

         srpms = glob.glob('%s/*.src.rpm' % broot.resultdir())
         if len(srpms) == 0:

On Fri, Aug 15, 2014 at 12:10 AM, Mike McLean <mikem(a)redhat.com&gt; wrote:
>  On 08/13/2014 06:05 PM, Mike McLean wrote:
> > This much gets you halfway there, but it looks like the system is just
> > going to choke later on having the sources under SOURCES instead of the
> > git root dir.
> >
> > I guess you could set up a source_cmd to move or link the sources
> > around, but that's hardly a clean solution.
> >
> > I wouldn't mind teaching koji to handle this layout, but it looks like
> > it might be a little more involved than just the spec portion (unless
> > I'm missing something). Did you have a solution for the SOURCES dir?
>
>  Actually not that complicated after all. I did add an option to turn the
>  support off in case it causes a problem (possible that someone could
>  have a SOURCES dir in their checkout for some other reason).
>
>  Anyway, here is where I'm at. Seems to be working.
>  


-- 
Thomas

[Mike McLean]

On 10/22/2014 03:55 AM, Thomas wrote:
>  Hi All,
>  
>  I updated your patch so it does not assume sourcedir=chrootdir
>  But it's not yet the perfect solution. 
This looks reasonable, except that you dropped the directory existence
check, which makes the code /require/ the new layout if
support_rpm_source_layout is enabled. This is something of an interim
solution and I'd rather not break things for other folks (esp since
support_rpm_source_layout defaults to True).

I think the fix is something like this:


>  For reference the layout is:
>  SPEC/
>  SOURCES/
>  .metadata-file
>  
>  diff --git a/builder/kojid b/builder/kojid
>  index 821f19d..4210823 100755
>  --- a/builder/kojid
>  +++ b/builder/kojid
>  @@ -455,6 +455,10 @@ class BuildRoot(object):
>  
>       def build_srpm(self, specfile, sourcedir, source_cmd):
>           self.session.host.setBuildRootState(self.id,'BUILDING')
>  +        if self.options.support_rpm_source_layout:
>  +            sources_dir = "%s/SOURCES" % sourcedir
>  +       else:
>  +           sources_dir = sourcedir
>           if source_cmd:
>               # call the command defined by source_cmd in the chroot so
>  any required files not stored in
>               # the SCM can be retrieved
>  @@ -466,7 +470,7 @@ class BuildRoot(object):
>                   self.expire()
>                   raise koji.BuildError, "error retrieving sources, %s"
>  % self._mockResult(rv)
>  
>  -        args = ['--no-clean', '--buildsrpm', '--spec',
> specfile,
>  '--sources', sourcedir,
>  +        args = ['--no-clean', '--buildsrpm', '--spec',
> specfile,
>  '--sources', sources_dir,
>                   '--target', 'noarch']
>  
>           rv = self.mock(args)
>  @@ -3793,10 +3797,7 @@ class BuildSRPMFromSCMTask(BaseBuildTask):
>  
>           #build srpm
>           self.logger.debug("Running srpm build")
>  -        sources_dir = "%s/SOURCES" % sourcedir
>  -        if not self.options.support_rpm_source_layout or not
>  os.path.isdir(sources_dir):
>  -            sources_dir = sourcedir
>  -        broot.build_srpm(spec_file, sources_dir, scm.source_cmd)
>  +        broot.build_srpm(spec_file, sourcedir, scm.source_cmd)
>  
>           srpms = glob.glob('%s/*.src.rpm' % broot.resultdir())
>           if len(srpms) == 0:
>  
>  On Fri, Aug 15, 2014 at 12:10 AM, Mike McLean <mikem(a)redhat.com&gt; wrote:
> > On 08/13/2014 06:05 PM, Mike McLean wrote:
> >> This much gets you halfway there, but it looks like the system is just
> >> going to choke later on having the sources under SOURCES instead of the
> >> git root dir.
> >>
> >> I guess you could set up a source_cmd to move or link the sources
> >> around, but that's hardly a clean solution.
> >>
> >> I wouldn't mind teaching koji to handle this layout, but it looks like
> >> it might be a little more involved than just the spec portion (unless
> >> I'm missing something). Did you have a solution for the SOURCES dir?
> >
> > Actually not that complicated after all. I did add an option to turn the
> > support off in case it causes a problem (possible that someone could
> > have a SOURCES dir in their checkout for some other reason).
> >
> > Anyway, here is where I'm at. Seems to be working.
> >
>  
>  
>