David Hendricks has posted comments on this change. ( https://review.coreboot.org/19206 )
Change subject: Convert flashrom to git
......................................................................
Patch Set 4:
(7 comments)
I'll try to split out the complex getrevision.sh stuff in the next patch set.
https://review.coreboot.org/#/c/19206/4/Makefile
File Makefile:
https://review.coreboot.org/#/c/19206/4/Makefile@a1374
PS4, Line 1374:
> Why didn't I bikeshed this for being totally unrelated?
Added back in.
https://review.coreboot.org/#/c/19206/4/Makefile@1396
PS4, Line 1396: @tar -cz --format=ustar -f $(EXPORTDIR)/flashrom-$(RELEASENAME).tar.gz -C $(EXPORTDIR)/ \
: $(TAROPTIONS) flashrom-$(RELEASENAME)/
: # Delete the exported directory again because it is most likely what's expected by the user.
: @rm -rf $(EXPORTDIR)/flashrom-$(RELEASENAME)
> Missing quotes around everything with $(RELEASENAME).
Done
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh
File util/getrevision.sh:
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@66
PS1, Line 66: "to the commit history from an upstream repository. If no git remote"\
> Looks good, but did you test it? Using echo this way there shouldn't be any
Obviously not, heh...
Going back to the original formatting, more or less, with shorter lines and line breaks.
It's still a lot of text, but at least now it's broken apart so each explanation and suggested action is clear. Here's how it looks in all three cases now, assuming Gerrit doesn't botch it...
-------------
To produce useful version information the build process needs access
to the commit history from an upstream repository. If no git remote
is pointing to one we can store the necessary information out of
sight and update it on every build.
To enable this functionality and fetch the upstream commits from
https://review.coreboot.org/flashrom.git
please execute the following:
git config flashrom.offline-builds false
Alternatively, add one of the upstream repositories as a git remote
to rely on that information.
If you want to work completely offline and generate possibly
meaningless version strings then disable it using:
git config flashrom.offline-builds true
You can force updating the local commit cache using:
./util/getrevision.sh --force-update-upcache
---------------
Fetching commit history from upstream repository:
https://review.coreboot.org/flashrom.git
To disable any network activity execute:
git config flashrom.offline-builds true
---------------
Fetching commit history from upstream is disabled - version
strings might be misleading.
To ensure proper version strings and allow network access execute:
git config flashrom.offline-builds false
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@69
PS1, Line 69:
> Why not? The message should draw the user's attention and isn't
Makes sense. Added the redirection back in.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@269
PS1, Line 269:
> Then we don't need the `if`.
Done
https://review.coreboot.org/#/c/19206/4/util/getrevision.sh
File util/getrevision.sh:
https://review.coreboot.org/#/c/19206/4/util/getrevision.sh@72
PS4, Line 72: "$upstream_url"\
> Maybe indent it a little.
Done
https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push
File util/git-hooks/pre-push:
https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@53
PS1, Line 53: "$commit" ]; then
> Sounds better but is not what I meant: Both have to be present.
I reworded this to try and make it less confusing. LMK what you think.
--
To view, visit https://review.coreboot.org/19206
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2
Gerrit-Change-Number: 19206
Gerrit-PatchSet: 4
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com>
Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: david.hendricks(a)gmail.com
Gerrit-Comment-Date: Mon, 19 Jun 2017 01:22:30 +0000
Gerrit-HasComments: Yes
Hello Stefan Tauner, david.hendricks(a)gmail.com, Philippe Mathieu-Daudé, Paul Menzel, build bot (Jenkins), Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/19206
to look at the new patch set (#5).
Change subject: Convert flashrom to git
......................................................................
Convert flashrom to git
- Drop support for Subversion in the getrevision script and Makefile.
- Add .gitignore and .gitattributes file (the latter to limit exports).
- Restore modification dates of the exported files from the SCM.
- Stop exporting SCM log dumps to CHANGELOG. This makes no sense.
- Remove djgpp-dos target (it is not different to other x-compilations).
- Do not export the pre-"compiled" manpage. It can be generated like
anything else from the code dump when we export the respective variable.
The latter is added with this change.
- Add some initial client-side git hooks
* When committing check for obvious stuff you never want anyway:
- white space errors
* When pushing to the upstream repository check mandatory rules:
- existing signoffs and acks in all new commits
- no deletions or creation of branches
- do not rewrite history of the precious branches, even if forced
- Change version string of flashrom as follows.
Previously, we included the last stable version according to a hard-
coded string in the Makefile and appended the subversion revision number.
With this patch the version string includes the last reachable git tag,
number of commits since said tag in the upstream branches (if any),
the name of said upstream branch, number of commits since that branch
(if any), and the shortened git hash.
In case there are uncommitted changes a "-dirty" indicator is also added.
The case of unknown versions is explicitly handled in getrevision instead
of simply appending "-unknown" to a hardcoded release number.
The version information is either taken from an existing git remote
pointing to an upstream repository (or a known mirror), or if that
is not available - with the user's consent - a shadow copy is fetched
from the upstream repo that is updated on every build (usually takes
less than a second).
In the following some examples of the version string changes are shown.
Basically we print the distance to the last known upstream tag, which
comprises any upstream commits since that tag as well as local commits on
top of that. Additionally we handle upstream's stable and staging branches
specially.
Old output:
flashrom v0.9.7-r1716 on Linux 3.8.0-6-generic (x86_64)
New output variants:
Build of the 0.9.99 release without any changes:
flashrom v0.9.99-e4f6643 on Linux 3.13.0-76-generic (x86_64)
5 commits since last tag in upstream's stable branch:
flashrom v0.9.99-5-stable-e4f6643-dirty on Linux 3.13.0-76-generic (x86_64)
3 commits since last tag in upstream's staging branch and
4 local commits on top of that:
flashrom v0.9.99-3-staging-4-e4f6643 on Linux 3.13.0-76-generic (x86_64)
3 commits since last tag in upstream's staging branch and
4 local commits on top of that, and some local uncommitted changes too:
flashrom v0.9.99-3-staging-4-e4f6643-dirty on Linux 3.13.0-76-generic (x86_64)
3 commits since the last tag in an unrelated upstream branch
(e.g., a stable release *branch* such as 0.9.99.x) or local branch:
flashrom v0.9.99-3-e4f6643 on Linux 3.13.0-76-generic (x86_64)
No tags reachable from current commit (generic git fallback):
flashrom d95935a version on Linux 3.13.0-76-generic (x86_64)
Not in a repository:
flashrom unknown version on Linux 3.13.0-76-generic (x86_64)
NOTE: This patch is adapted from Stefan Tauner's original commit (
f5dd7ce1), but uses coreboot's commit-msg hook which includes support
for generating and appending Change-Id which is now mandatory when
pushing to the review server.
Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2
Signed-off-by: Stefan Tauner <stefan.tauner(a)alumni.tuwien.ac.at>
Signed-off-by: David Hendricks <dhendricks(a)fb.com>
---
A .gitattributes
A .gitignore
M Makefile
M flashrom.c
M util/getrevision.sh
A util/git-hooks/applypatch-msg
A util/git-hooks/commit-msg
A util/git-hooks/install.sh
A util/git-hooks/pre-applypatch
A util/git-hooks/pre-commit
A util/git-hooks/pre-push
A util/git-hooks/wrapper.sh
12 files changed, 582 insertions(+), 125 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/19206/5
--
To view, visit https://review.coreboot.org/19206
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2
Gerrit-Change-Number: 19206
Gerrit-PatchSet: 5
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com>
Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: david.hendricks(a)gmail.com
Nico Huber has posted comments on this change. ( https://review.coreboot.org/19206 )
Change subject: Convert flashrom to git
......................................................................
Patch Set 4:
(10 comments)
Looks pretty good overall, except for the complex getrevision.sh
changes. Splitting it out into a follow-up would get the other
changes in (with a minor update, see inline comments), I suppose.
https://review.coreboot.org/#/c/19206/4/Makefile
File Makefile:
https://review.coreboot.org/#/c/19206/4/Makefile@a1374
PS4, Line 1374:
Why didn't I bikeshed this for being totally unrelated?
https://review.coreboot.org/#/c/19206/4/Makefile@1396
PS4, Line 1396: @tar -cz --format=ustar -f $(EXPORTDIR)/flashrom-$(RELEASENAME).tar.gz -C $(EXPORTDIR)/ \
: $(TAROPTIONS) flashrom-$(RELEASENAME)/
: # Delete the exported directory again because it is most likely what's expected by the user.
: @rm -rf $(EXPORTDIR)/flashrom-$(RELEASENAME)
Missing quotes around everything with $(RELEASENAME).
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh
File util/getrevision.sh:
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@66
PS1, Line 66: "to the commit history from an upstream repository. If no git remote"\
> Done
Looks good, but did you test it? Using echo this way there shouldn't be any line breaks in the output at all.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@69
PS1, Line 69:
> not needed?
Why not? The message should draw the user's attention and isn't
usual program output (i.e. make progress).
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@224
PS1, Line 224: urn
> I'm wondering if this is all too elaborate. I'd rather keep it simple and s
Sounds good, I was all for splitting from the beginning ;)
We could just update getrevision.sh to use git for a start.
Add the upcache + complex version string (we should start a
discussion on the ML probably) later.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@224
PS1, Line 224: return
: fi
: done
: }
:
: revision() {
: local sha=$(git_last_commit "$1" 2>/dev/null)
: #
> What we'd need here instead is
What I meant here is roughly: In the `-n "$up_remote"` case,
we only construct $up_refs based on registered remotes and
$upstream_branches. If the user just registered the upstream
remote without ever fetching it, $up_refs will contain refs
unknown to git.
What we need to do is kind of force_update_upcache but for
the detected already present remotes. We could spare us all
that hassle and some of the already written code if we'd
always use an upcache.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@257
PS1, Line 257: update_upcache || { echo "offline" ; return ; }
: up_refs=$upcache_refs
: fi
:
: # Find nearest commit contained in this branch that is also in any of the up_refs, i.e. the branch point
: # of the current branch. This might be the latest commit if it's in any of the upstream branches.
: loca
> TBH, I'm not sure what to do about that here :-/
Don't worry. It's only a cosmetic issue. And I don't remember it accurately.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@269
PS1, Line 269:
> Yep, now it is!
Then we don't need the `if`.
https://review.coreboot.org/#/c/19206/4/util/getrevision.sh
File util/getrevision.sh:
https://review.coreboot.org/#/c/19206/4/util/getrevision.sh@72
PS4, Line 72: "$upstream_url"\
Maybe indent it a little.
https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push
File util/git-hooks/pre-push:
https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@53
PS1, Line 53: "$commit" ]; then
> If we're going that route, "Neither Signoff nor Ack were found..." would so
Sounds better but is not what I meant: Both have to be present.
--
To view, visit https://review.coreboot.org/19206
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2
Gerrit-Change-Number: 19206
Gerrit-PatchSet: 4
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com>
Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: david.hendricks(a)gmail.com
Gerrit-Comment-Date: Sat, 17 Jun 2017 13:31:43 +0000
Gerrit-HasComments: Yes