attached is a patch that attempts to generalize checking out the components used to build an SRPM. this patch supports CVS, GIT, and SVN, but only CVS and SVN have been tested. the idea is to provide the infrastructure for different SCM systems to be configured at run-time so that users can choose their favorite system.
is there a better approach? did i miss something obvious? general comments?
thanks.
rob.
On Tue, 2007-10-23 at 14:13 -0400, rob myers wrote:
attached is a patch that attempts to generalize checking out the components used to build an SRPM. this patch supports CVS, GIT, and SVN, but only CVS and SVN have been tested. the idea is to provide the infrastructure for different SCM systems to be configured at run-time so that users can choose their favorite system.
is there a better approach? did i miss something obvious? general comments?
It looks pretty good, but I wonder if you need to make scmtype a config option? I'd rather have Koji support all scmtypes all the time and have some other method for restricting what URLs are valid locations to download the source from. Maybe a list of valid hostnames or hostname/path pairs?
On Tue, 2007-10-23 at 14:25 -0400, Mike Bonnet wrote:
On Tue, 2007-10-23 at 14:13 -0400, rob myers wrote:
attached is a patch that attempts to generalize checking out the components used to build an SRPM. this patch supports CVS, GIT, and SVN, but only CVS and SVN have been tested. the idea is to provide the infrastructure for different SCM systems to be configured at run-time so that users can choose their favorite system.
is there a better approach? did i miss something obvious? general comments?
It looks pretty good, but I wonder if you need to make scmtype a config option? I'd rather have Koji support all scmtypes all the time and have some other method for restricting what URLs are valid locations to download the source from. Maybe a list of valid hostnames or hostname/path pairs?
what do you mean by "path"? /cvsroot or /usr/bin/cvs?
i added the scmtype to function as a mechanism to determine which binary to call for a given url. for example, both git and svn can have http:// urls, so the url itself does not currently contain enough information to indicate which binary should be called. at the same time, there is no reason koji could not support all types of scm at once- if there is a mechanism to determine what binary or checkout command to call for a given url.
an acl of approved SCM backends seems like it would be a nice feature.
rob.
On Tue, 2007-10-23 at 14:13 -0400, rob myers wrote:
attached is a patch that attempts to generalize checking out the components used to build an SRPM. this patch supports CVS, GIT, and SVN, but only CVS and SVN have been tested. the idea is to provide the infrastructure for different SCM systems to be configured at run-time so that users can choose their favorite system.
It would also be nice if rather than assuming the existence of the "common/" module and hard-coding the "make srpm" command, there was some way to specify what modules were necessary to checkout, and what command to run to create a srpm. This should probably be configurable per-repository. It might make sense to add a new database table to store this information in a more flexible way than possible with the config files.
However, this is a great start. I'm going to review the patch more carefully, and I'll see about merging it soon (after possibly removing the scmtype config option I mentioned earlier).
Thanks a lot!
On Tue, 2007-10-23 at 14:30 -0400, Mike Bonnet wrote:
On Tue, 2007-10-23 at 14:13 -0400, rob myers wrote:
attached is a patch that attempts to generalize checking out the components used to build an SRPM. this patch supports CVS, GIT, and SVN, but only CVS and SVN have been tested. the idea is to provide the infrastructure for different SCM systems to be configured at run-time so that users can choose their favorite system.
It would also be nice if rather than assuming the existence of the "common/" module and hard-coding the "make srpm" command, there was some way to specify what modules were necessary to checkout, and what command to run to create a srpm. This should probably be configurable per-repository. It might make sense to add a new database table to store this information in a more flexible way than possible with the config files.
the existing design is (currently) sufficient for my needs. what you describe is more flexible and is probably a better design, but is it needed?
However, this is a great start. I'm going to review the patch more carefully, and I'll see about merging it soon (after possibly removing the scmtype config option I mentioned earlier).
thanks for your feedback, and remove cruft as necessary. :)
rob.
On Tue, 2007-10-23 at 15:14 -0400, rob myers wrote:
On Tue, 2007-10-23 at 14:30 -0400, Mike Bonnet wrote:
On Tue, 2007-10-23 at 14:13 -0400, rob myers wrote:
attached is a patch that attempts to generalize checking out the components used to build an SRPM. this patch supports CVS, GIT, and SVN, but only CVS and SVN have been tested. the idea is to provide the infrastructure for different SCM systems to be configured at run-time so that users can choose their favorite system.
It would also be nice if rather than assuming the existence of the "common/" module and hard-coding the "make srpm" command, there was some way to specify what modules were necessary to checkout, and what command to run to create a srpm. This should probably be configurable per-repository. It might make sense to add a new database table to store this information in a more flexible way than possible with the config files.
the existing design is (currently) sufficient for my needs. what you describe is more flexible and is probably a better design, but is it needed?
However, this is a great start. I'm going to review the patch more carefully, and I'll see about merging it soon (after possibly removing the scmtype config option I mentioned earlier).
thanks for your feedback, and remove cruft as necessary. :)
The patch has been committed to Koji:
http://git.fedoraproject.org/?p=hosted/koji;a=commit;h=27ac942683d8bbfd28c3b...
I made some pretty significant changes to make the SCM class more object-y, remove some code duplication, make it more configurable at runtime, etc., but the framework setup in the original patch is there. I've tested it fairly completely and everything seems to be working as expected. Note that when using the "+ssh://" access methods, you're responsible for getting the required login credentials (host keys, private keys, Kerberos tickets) into the right place on the builders yourself.
Thanks for the patch Rob, and if you have any other suggestions or improvements, please let me know.
Keep those patches coming! :)
On Mon, 2007-11-05 at 18:56 -0500, Mike Bonnet wrote:
The patch has been committed to Koji:
http://git.fedoraproject.org/?p=hosted/koji;a=commit;h=27ac942683d8bbfd28c3b...
I made some pretty significant changes to make the SCM class more object-y, remove some code duplication, make it more configurable at runtime, etc., but the framework setup in the original patch is there. I've tested it fairly completely and everything seems to be working as expected. Note that when using the "+ssh://" access methods, you're responsible for getting the required login credentials (host keys, private keys, Kerberos tickets) into the right place on the builders yourself.
mike-
your patch is much tidier than mine, thanks for fixing it up! i really like the removal of scmtype from the configuration file. i'm mixed on the removal of scmusername because i'm not sure that koji clients should specify the username used by the koji builder to perform ssh checkouts. however, while i thought it worth pointing out, it does not seem too big a deal to disclose this username.
i did some testing with my svn+ssh repository and found the patch below necessary- it creates ../common only if it does not already exist.
diff --git a/builder/kojid b/builder/kojid index b436a19..5c5d665 100755 --- a/builder/kojid +++ b/builder/kojid @@ -2506,7 +2507,8 @@ class SCM(object): (self.scmtype, ' '.join(update_checkout_cmd), os.path.basename(logfile))
# Required by Dist CVS layout - os.symlink('%s/common' % scmdir, '%s/../common' % sourcedir) + if not os.path.exists('%s/../common' % sourcedir): + os.symlink('%s/common' % scmdir, '%s/../common' % sourcedir)
def get_options(): """process options from command line and config file"""
thanks again for adding this functionality to koji. :)
rob.
On Tue, 2007-11-06 at 11:16 -0500, rob myers wrote:
On Mon, 2007-11-05 at 18:56 -0500, Mike Bonnet wrote:
The patch has been committed to Koji:
http://git.fedoraproject.org/?p=hosted/koji;a=commit;h=27ac942683d8bbfd28c3b...
I made some pretty significant changes to make the SCM class more object-y, remove some code duplication, make it more configurable at runtime, etc., but the framework setup in the original patch is there. I've tested it fairly completely and everything seems to be working as expected. Note that when using the "+ssh://" access methods, you're responsible for getting the required login credentials (host keys, private keys, Kerberos tickets) into the right place on the builders yourself.
mike-
your patch is much tidier than mine, thanks for fixing it up! i really like the removal of scmtype from the configuration file. i'm mixed on the removal of scmusername because i'm not sure that koji clients should specify the username used by the koji builder to perform ssh checkouts. however, while i thought it worth pointing out, it does not seem too big a deal to disclose this username.
i did some testing with my svn+ssh repository and found the patch below necessary- it creates ../common only if it does not already exist.
diff --git a/builder/kojid b/builder/kojid index b436a19..5c5d665 100755 --- a/builder/kojid +++ b/builder/kojid @@ -2506,7 +2507,8 @@ class SCM(object): (self.scmtype, ' '.join(update_checkout_cmd), os.path.basename(logfile))
# Required by Dist CVS layout
os.symlink('%s/common' % scmdir, '%s/../common' % sourcedir)
if not os.path.exists('%s/../common' % sourcedir):os.symlink('%s/common' % scmdir, '%s/../common' %sourcedir)
def get_options(): """process options from command line and config file"""
thanks again for adding this functionality to koji. :)
The patch looks good, I've applied it. Just out of curiousity, where did the common/ directory come from, an svn:external?
On Tue, 2007-11-06 at 15:33 -0500, Mike Bonnet wrote:
On Tue, 2007-11-06 at 11:16 -0500, rob myers wrote:
i did some testing with my svn+ssh repository and found the patch below necessary- it creates ../common only if it does not already exist.
diff --git a/builder/kojid b/builder/kojid index b436a19..5c5d665 100755 --- a/builder/kojid +++ b/builder/kojid @@ -2506,7 +2507,8 @@ class SCM(object): (self.scmtype, ' '.join(update_checkout_cmd), os.path.basename(logfile))
# Required by Dist CVS layout
os.symlink('%s/common' % scmdir, '%s/../common' % sourcedir)
if not os.path.exists('%s/../common' % sourcedir):os.symlink('%s/common' % scmdir, '%s/../common' %sourcedir)
def get_options(): """process options from command line and config file"""
The patch looks good, I've applied it. Just out of curiousity, where did the common/ directory come from, an svn:external?
nope nothing fancy; it is just another dir sitting at the same level as all the packages.
thanks again.
rob.
buildsys@lists.fedoraproject.org
-
Mike Bonnet -
Rob Myers