Ticket #959 (closed enhancement: fixed)

Opened 3 years ago

Last modified 3 years ago

Add 'native' CVS support

Reported by: AHowell Owned by: AHowell
Priority: major Milestone: 0.8.2
Version: master Keywords: cvs
Cc:

Description

Add support for CVS with script in contrib. I had problems finding and/or making the methods in the docs work.

I will submit scripts and docs to do this. A script is called by CVS at checkin. This generates a mail the is picked up by a new source in changes/mail.py. The source has a hook to add URLs for the changed files. It also adds a property to the Change that lists both the files and their old and new versions. I'm not sure if that should stay in there or not.

Change History

comment:1 Changed 3 years ago by dustin

  • Keywords cvs added
  • Milestone changed from undecided to 0.8.2

I'm not sure what "native" means here -- can you tell me more about what is inadequate in the current setup? Is it just documentation?

I've absolutely no opposition to improving things - I just want to be clear on the goal :)

comment:2 Changed 3 years ago by AHowell

Sure, sorry that was not very descriptive. I had problems tracking down a working change notification method.

FreshCVS - Could not get this going at all, project dead? Bonsai - Tried this awhile ago, can't remember why I didn't use it. syncmail - Works, but could not determine repo. repository not set. No updates since 05

I sent some mail on it previously.  http://sourceforge.net/mailarchive/message.php?msg_name=4C345020.3010903%40austin.rr.com

By 'native', I meant CVS support that is shipped with buildbot. With in the distribution, its more likely to be supported in the future. It also makes it easier to deploy buildbot, as its one less external package to track down. The changes to the docs have more detail how it works etc. I'm happy to elaborate if it doesn't make sense. Thanks.

comment:3 Changed 3 years ago by dustin

Thanks - that's exactly what I was looking for. Let's remove the cruft while we're at it.

The new stuff should have tests, too -- and tests that don't depend on an actual CVS installation. I've done a bit of work to that end with the slave-side source tests, if you want to have a look.

comment:4 Changed 3 years ago by AHowell

De-cruffted and pushed cvsmailsouce branch

The patch involves files under master/builbot

changes/mail.py

-- new class BuildbotCVSMaildirSource

contrib/buildbot_cvs_mail.py

-- submits mail with changes

test/unit/test_contrib_buildbot_cvs_mail.py

-- test contrib/buildbot_cvs_mail.py

test/unit/test_changes_mail_MailDirSource.py

-- test new BuildbotCVSMaildirSource class

Under master/docs

cfg-changesources.texinfo

-- Explain how to install and use above

comment:5 Changed 3 years ago by AHowell

Dustin, you said: "Let's remove the cruft while we're at it."

I took that to mean removing my unused code (fileTupleList) in mail.py. Were you referring to the other cvs MaildirSources?? I don't know which, if any, are in current use. I'd be hesitant to remove/change them.

comment:6 Changed 3 years ago by dustin

I was referring to the other, no-longer-maintained stuff, on the basis that it's just extra code we're trying to maintain. But maybe it's best to just leave it.

Is there anything else you're planning to do here? Thanks!

comment:7 Changed 3 years ago by AHowell

Maybe the approach take is to mark the existing CVS MailDirSources? as legacy in the docs and release notes, saying they may be removed in a future release. If thats acceptable, I'll modify the docs.

I think my code is functionally complete for most needs. The two attributes of a Change that it does not set are 'revlink' and 'properties'. 'revlink' does not doesn't make sense for CVS since it does not have an id for a commit. I get around that by having a user supplied function to map each file name, old CVS revision, new CVS revision to a URL. Hmm, I should have called it 'urlmapper' instead of 'urlmaker'.

Perhaps I should add the 'properties' attribute, so they can be specified as part of a BuildbotCVSMaildirSource. 'properties' in this case is a dictionary, not a Properties class. That is the only tweak I can think of.

comment:8 Changed 3 years ago by dustin

I think that a deprecation is a good idea. Please also add it to the NEWS section, and ask users of the old class to contact the mailing list if they would like to see the old classes stick around.

As for Change attributes - 'properties' would be a good addition, and as far as I can see 'repository' is not set, either.

Before you work on that, though, I'm working on some changes that I have pushed to

 http://github.com/djmitche/buildbot/commits/bug959

In particular:

  • rename BuildbotCVSMaildirSource to CVSMaildirSource (since the "Buildbot" part is redundant within Buildbot itself)
  • use Python's optparse (available since python-2.3) instead of manually parsing arguments

At the moment, the tests are passing even though the optparse stuff fails, so I need to figure out what's wrong with the tests, and then fix the optparsing.

comment:9 Changed 3 years ago by AHowell

Thanks for fixing the option parsing. I'd do the grunt work if you pointed me in the right direction. :)

Should I wait for the test fixes?

I need some git hand-holding. How do I update my cvsmailsource branch? I tried doing a fetch:

git fetch --dry-run  http://github.com/djmitche/buildbot/commits/bug959

fatal:  http://github.com/djmitche/buildbot/commits/bug959/info/refs not found: did you run git update-server-info on the server?

Thanks

comment:10 Changed 3 years ago by dustin

OK, I'm done with the changes on my branch, so you can take it back. As for fetching from a remote repository, there are a few small things you're getting wrong. First, the URL you're using is a regular HTML page - you want to use  git://github.com/djmitche/buildbot.git instead. Second, fetch is designed to bring a 'remote' up to date, and that might be overkill. Here's how you would set up that remote, though:

git remote add djmitche git://github.com/djmitche/buildbot.git
git fetch djmitche

However, since all you want to do is pull my changes into your branch, you can just do this:

git pull git://github.com/djmitche/buildbot.git bug959

No fuss, no mas. The command will do a fast-forward in this particular case - meaning it just appends my commits to your commits - but if necessary it will perform a merge. This is, in fact, how I pull others' buildbot patches into the master repository.

comment:11 Changed 3 years ago by AHowell

Thanks a bunch. I've pushed my changes. I added a number of tests to the both the mail.py side and buildbot_cvs_mail.py unit tests.

I've updated the docs, adding note about the depreciation. I noticed that the mozilla people are using bonsai, but it looks like they are using tinderbox for the builds. Not sure if we should depreciate bonsai support. A long time ago I had a tinderbox setup here. When the box it was on died, I couldn't find updated source for it, so I moved to buildbot! Every day I can avoid perl is a good day.

I wasn't sure about your comment in buildbot_cvs_mail.py: # TODO: make sure tests really run

Regarding the setting of the 'repository', that is set by the --cvsroot option.

comment:12 Changed 3 years ago by Dustin J. Mitchell

  • Status changed from new to closed
  • Resolution set to fixed

Merge branch 'cvsmailsource' of  git://github.com/AndyHowell/buildbot

Fixes #959

  • 'cvsmailsource' of  git://github.com/AndyHowell/buildbot: option.smtplib was not getting when running live Added depreciation notes to news and manual Added more test to test_changes_mail_CVSMaildirSource.py and test_contrib_buildbot_cvs_mail.py Fixed mail.py problems brought out by tests use optparse for buildbot_cvs_mail; fix its tests rename to CVSMaildirSource fix relative path handling in buildbot_cvs_mail tests docs tweaks for cvs support buildbot/changes/mail.py buildbot/test/unit/test_changes_mail_MaildirSource.py removed fileTupleList as it was not being used master/buildbot/changes/mail.py master/contrib/buildbot_cvs_mail.py Replaced tabs with spaces master/buildbot/changes/mail.py added BuildbotCVSMaildir source master/buildbot/test/unit/test_changes_mail_MaildirSource.py test for BuildbotCVSMaildir in mail.py master/buildbot/test/unit/test_contrib_buildbot_cvs_mail.py test for buildbot_cvs_mail.py master/contrib/buildbot_cvs_mail.py send mail to buildbot on CVS checkins master/docs/cfg-changesources.texinfo updated docs Fixes #951. Web rss feed is choking on UTF-8 log data Changeset: b9feff0338ecd7cceead5ede70852abc1f0a6d2a

comment:13 Changed 3 years ago by dustin

Committed, as you can see. I'll remove that comment - it was a brain-dump when I got up from the keyboard.

comment:14 Changed 3 years ago by AHowell

Thanks! I forgot to mention I added the ability to set properties in CVSMaildirSource. I saw that you made the retrieving of the user name failsafe. The buildbot_cvs_mail.py script won't run on windows, as it forks for sending the mail. The forks so that CVS can continue on. I don't have much expertise in windows, nor I don't imagine there are many users running the CVS repository under windows either; I'm leaving it as is for now.

The other 'deficiency' with it stems from the fact that when you check in files from multiple directories, each directory generates a separate mail. This shows up in the waterfall as multiple changes, each with the same comment, but different list of files, and possible a slightly different revision time. Annoying but usable.

After my short experience with GIT, I'm ready to switch to that. I'm stuck with CVS though, so hopefully this will help others in a similar situation. Thanks for all all your help adding this.

comment:15 Changed 3 years ago by dustin

Yes, I needed to get the tests to pass on Windows. I think smtplib actually uses smtp, rather than forking, so it should work. But you're right - running a CVS master on windows is rare, if it ever happens :)

As for CVS's inability to define a single "change" properly, that's life..

Git is pretty cool, for sure!

Note: See TracTickets for help on using tickets.