Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Follow startup log using a stdout logger. #583

Closed
wants to merge 2 commits into from

Conversation

tomprince
Copy link
Member

No description provided.

@@ -51,10 +51,15 @@ def run(self, basedir, quiet):
self.sent_signal = False
reactor.callLater(0.2, self.sighup)

lw = LogWatcher(os.path.join(basedir, "twistd.log"))
lw = LogWatcher()
self.process = reactor.spawnProcess(lw.pp, "/usr/bin/tail",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit this has been in the codebase forever and hasn't caused any problems, but I cringe at shelling out to 'tail' instead of implementing this natively.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing at at time. ;)

@djmitche
Copy link
Member

Untested, but this looks great. If there's a requirement to put _startupLogger.py in the package root, rather than in master/buildbot/util or something like that, then it's fine.

@tomprince
Copy link
Member Author

This could certainly use more tests, but buildbot.test.unit.test_scripts_start.test_start does exercise the buildbot start code.

@jaredgrubb
Copy link
Member

Looks ok to me, FWIW :)

@djmitche
Copy link
Member

djmitche commented Dec 1, 2012

Looks fine to me, too.

djmitche@pull583 moves _startupLogger.py without any apparent problems.

You may want to fold http://trac.buildbot.net/ticket/2405 in here, too?

@djmitche
Copy link
Member

@tomprince - want to get this merged?

@tomprince
Copy link
Member Author

Except, that still loads packages.

I think the correct thing to do, is simply say that this is an incompatible change.

@djmitche
Copy link
Member

djmitche commented Feb 7, 2013

Works for me..

mzdaniel pushed a commit to mzdaniel/buildbot that referenced this pull request Aug 2, 2013
* Api: Improve remote api unit tests
@djmitche
Copy link
Member

@tomprince - do you want to land this for 0.8.9?

@ghost ghost assigned tomprince Dec 8, 2013
@djmitche
Copy link
Member

djmitche commented Apr 7, 2014

This needs to be updated to merge, but is otherwise ready to go, i think.

@djmitche
Copy link
Member

djmitche commented May 5, 2014

Closing for now, but the code is here!

@djmitche djmitche closed this May 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants