Patchwork [RFC,v2,1/3] buildhistory: record all builds

login
register
mail settings
Submitter Koen Kooi
Date March 20, 2012, 1:38 p.m.
Message ID <1332250734-14660-1-git-send-email-koen@dominion.thruhere.net>
Download mbox | patch
Permalink /patch/23859/
State New
Headers show

Comments

Koen Kooi - March 20, 2012, 1:38 p.m.
Allow empty commits, this also give a nice speedup since 'git status --porcelain' doesn't need to get run.

Signed-off-by: Koen Kooi <koen@dominion.thruhere.net>
---
 meta/classes/buildhistory.bbclass |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)
Paul Eggleton - March 20, 2012, 1:51 p.m.
On Tuesday 20 March 2012 14:38:52 Koen Kooi wrote:
> Allow empty commits, this also give a nice speedup since 'git status
> --porcelain' doesn't need to get run.

I guess my thinking before (without any verification) was that git commit would 
need to be running the equivalent of 'git status --porcelain' anyway and the 
second time it would be cached so there wouldn't be much impact. If the 
buildhistory repo is quite large or the machine is busy then it's entirely 
possible that wouldn't work out however.

Since all image builds will force a commit anyway (as at least build-id must 
change), and for the majority of people most builds will be image builds, I 
think this is going to be a net improvement.

Therefore I'll say:

Acked-by: Paul Eggleton <paul.eggleton@linux.intel.com>
Koen Kooi - March 20, 2012, 2:08 p.m.
Op 20 mrt. 2012, om 14:51 heeft Paul Eggleton het volgende geschreven:

> On Tuesday 20 March 2012 14:38:52 Koen Kooi wrote:
>> Allow empty commits, this also give a nice speedup since 'git status
>> --porcelain' doesn't need to get run.
> 
> I guess my thinking before (without any verification) was that git commit would 
> need to be running the equivalent of 'git status --porcelain' anyway

It does

> and the 
> second time it would be cached so there wouldn't be much impact.

On my non-SSD machine the cache doesn't seem to be working out :(

> If the 
> buildhistory repo is quite large or the machine is busy then it's entirely 
> possible that wouldn't work out however.

Exactly!

regards,

Koen
Koen Kooi - March 22, 2012, 10:35 a.m.
Op 20 mrt. 2012, om 14:51 heeft Paul Eggleton het volgende geschreven:

> On Tuesday 20 March 2012 14:38:52 Koen Kooi wrote:
>> Allow empty commits, this also give a nice speedup since 'git status
>> --porcelain' doesn't need to get run.
> 
> I guess my thinking before (without any verification) was that git commit would 
> need to be running the equivalent of 'git status --porcelain' anyway and the 
> second time it would be cached so there wouldn't be much impact. If the 
> buildhistory repo is quite large or the machine is busy then it's entirely 
> possible that wouldn't work out however.
> 
> Since all image builds will force a commit anyway (as at least build-id must 
> change), and for the majority of people most builds will be image builds, I 
> think this is going to be a net improvement.
> 
> Therefore I'll say:
> 
> Acked-by: Paul Eggleton <paul.eggleton@linux.intel.com>

After a few more days of usage I'm going to create a follow-up patch that will do:

1) reinstate the global git status --porcelain and make a single, empty commit if nothing changed instead of N empty commits.
2) either parse the output of git status or re-run it to see which top level entries need to get committed and only run 'git add ; git commit' on those.

That will keep tracking all builds, but will be less noisy. But it will be slightly slower when the OS doesn't cache the git status, but I decided that I can personally live with that.

So what do you think about that? If you like the idea, would you like it as follow up or as a respin of the series?

regards,

Koen
Paul Eggleton - March 22, 2012, 2:25 p.m.
On Thursday 22 March 2012 11:35:30 Koen Kooi wrote:
> After a few more days of usage I'm going to create a follow-up patch that
> will do:
> 
> 1) reinstate the global git status --porcelain and make a single, empty
> commit if nothing changed instead of N empty commits. 2) either parse the
> output of git status or re-run it to see which top level entries need to
> get committed and only run 'git add ; git commit' on those.
>
> That will keep tracking all builds, but will be less noisy. But it will be
> slightly slower when the OS doesn't cache the git status, but I decided
> that I can personally live with that.
> 
> So what do you think about that? 

Well, avoiding >1 empty commits sounds good. I'm happy to go with your 
assessment as you're more likely to be observing actual performance than me 
(I'm not really monitoring the performance of buildhistory on our autobuilder, 
only the output).

FYI I didn't comment on the splitting into separate commits patch earlier 
because it doesn't really bother me either way. I do think people will find it 
easier to use the buildhistory-diff tool rather than looking at the git log 
directly (or the web-based equivalent when I finish that.); but I'm happy to 
continue supporting those that prefer to read the log.

> If you like the idea, would you like it as follow up or as a respin of the
> series?

Since it hasn't been merged I'll go for a respin, if you don't mind.

Cheers,
Paul
Martin Jansa - March 22, 2012, 2:33 p.m.
On Thu, Mar 22, 2012 at 02:25:38PM +0000, Paul Eggleton wrote:
> On Thursday 22 March 2012 11:35:30 Koen Kooi wrote:
> > After a few more days of usage I'm going to create a follow-up patch that
> > will do:
> > 
> > 1) reinstate the global git status --porcelain and make a single, empty
> > commit if nothing changed instead of N empty commits. 2) either parse the
> > output of git status or re-run it to see which top level entries need to
> > get committed and only run 'git add ; git commit' on those.
> >
> > That will keep tracking all builds, but will be less noisy. But it will be
> > slightly slower when the OS doesn't cache the git status, but I decided
> > that I can personally live with that.
> > 
> > So what do you think about that? 
> 
> Well, avoiding >1 empty commits sounds good. I'm happy to go with your 
> assessment as you're more likely to be observing actual performance than me 
> (I'm not really monitoring the performance of buildhistory on our autobuilder, 
> only the output).

Koen please make it to reuse output..

git status --porcelain takes ages here and even more on my 2nd buildhost
so even with 2nd run just after 1st probably faster I hope it will be
worth reparsing.

OE @ ~/shr-core/tmp-eglibc/buildhistory $ du -sh .
520M    .
and after git gc..
OE @ ~/shr-core/tmp-eglibc/buildhistory $ du -sh .
378M    .

> FYI I didn't comment on the splitting into separate commits patch earlier 
> because it doesn't really bother me either way. I do think people will find it 
> easier to use the buildhistory-diff tool rather than looking at the git log 
> directly (or the web-based equivalent when I finish that.); but I'm happy to 
> continue supporting those that prefer to read the log.

same here

> > If you like the idea, would you like it as follow up or as a respin of the
> > series?

Cheers,
Koen Kooi - March 22, 2012, 2:46 p.m.
Op 22 mrt. 2012, om 15:25 heeft Paul Eggleton het volgende geschreven:

> On Thursday 22 March 2012 11:35:30 Koen Kooi wrote:
>> After a few more days of usage I'm going to create a follow-up patch that
>> will do:
>> 
>> 1) reinstate the global git status --porcelain and make a single, empty
>> commit if nothing changed instead of N empty commits. 2) either parse the
>> output of git status or re-run it to see which top level entries need to
>> get committed and only run 'git add ; git commit' on those.
>> 
>> That will keep tracking all builds, but will be less noisy. But it will be
>> slightly slower when the OS doesn't cache the git status, but I decided
>> that I can personally live with that.
>> 
>> So what do you think about that? 
> 
> Well, avoiding >1 empty commits sounds good. I'm happy to go with your 
> assessment as you're more likely to be observing actual performance than me 
> (I'm not really monitoring the performance of buildhistory on our autobuilder, 
> only the output).
> 
> FYI I didn't comment on the splitting into separate commits patch earlier 
> because it doesn't really bother me either way. I do think people will find it 
> easier to use the buildhistory-diff tool rather than looking at the git log 
> directly (or the web-based equivalent when I finish that.); but I'm happy to 
> continue supporting those that prefer to read the log.
> 
>> If you like the idea, would you like it as follow up or as a respin of the
>> series?
> 
> Since it hasn't been merged I'll go for a respin, if you don't mind.

I'll respin and take Martins concerns in as well. Not sure when the new patchset will be sent, 2 weeks of tradeshows, bspfests and summits ahead :)

regards,

Koen

Patch

diff --git a/meta/classes/buildhistory.bbclass b/meta/classes/buildhistory.bbclass
index f99aa7f..1926d12 100644
--- a/meta/classes/buildhistory.bbclass
+++ b/meta/classes/buildhistory.bbclass
@@ -396,16 +396,13 @@  buildhistory_commit() {
 		if [ ! -d .git ] ; then
 			git init -q
 		fi
-		# Ensure there are new/changed files to commit
-		repostatus=`git status --porcelain`
-		if [ "$repostatus" != "" ] ; then
-			git add ${BUILDHISTORY_DIR}/*
-			HOSTNAME=`hostname 2>/dev/null || echo unknown`
-			git commit ${BUILDHISTORY_DIR}/ -m "Build ${BUILDNAME} of ${DISTRO} ${DISTRO_VERSION} for machine ${MACHINE} on $HOSTNAME" --author "${BUILDHISTORY_COMMIT_AUTHOR}" > /dev/null
-			if [ "${BUILDHISTORY_PUSH_REPO}" != "" ] ; then
-				git push -q ${BUILDHISTORY_PUSH_REPO}
-			fi
-		fi) || true
+		git add ${BUILDHISTORY_DIR}/*
+		HOSTNAME=`hostname 2>/dev/null || echo unknown`
+		git commit ${BUILDHISTORY_DIR}/ --allow-empty -m "Build ${BUILDNAME} of ${DISTRO} ${DISTRO_VERSION} for machine ${MACHINE} on $HOSTNAME" --author "${BUILDHISTORY_COMMIT_AUTHOR}" > /dev/null
+		if [ "${BUILDHISTORY_PUSH_REPO}" != "" ] ; then
+			git push -q ${BUILDHISTORY_PUSH_REPO}
+		fi
+	) || true
 }
 
 python buildhistory_eventhandler() {