Add OpenZFS auto merge script #51

gmelikov · 2017-01-25T16:40:12Z

This script can:

iterate all unmerged commits and try to merge it
create branch with changes on success
check for compile and cstyle errors
generate lists with commits in different state (compile errors, style errors, merge errors)

openzfs-tracking.sh now can generate text files with unmerged OpenZFS commits, if add -c parameter.
We can post them on website too for easier auto merge process.

This is a fast written draft, welcome any thoughts. I think it will be better to wait some time before merge it.

I tested it, you can see last PRs from my autoport branches.

Unfourtunately, there is no suitable to auto merge commit, you can add openzfs/openzfs@9185393 to hashes file manually.

Based on #50.

gmelikov · 2017-01-25T20:27:18Z

Add manual mode:

default - iterate commits and try to merge without manual interruption
-c - merge specified commit (by hash) without undo on failure. Makes branch with merge conflicts.
-g - generate commit description and git commit --amend it to last commit (by commit hash, in created by -c branch)

Now i can do:

./openzfs-merge.sh -d /path/to/zfs -c 40510e8 to begin cherry-picking
edit any found conflicts by git mergetool
git add/rm
git commit with any description
-./openzfs-merge.sh -d /path/to/zfs -g 40510e8 to end with description

Maybe it's an overkill, but it makes porting easier for me.

behlendorf · 2017-01-26T17:46:47Z

@gmelikov thanks, this looks really useful and your manual mode looks ideal for the use case I have in mind. Once we're largely caught up with the OpenZFS tree we're going to want to automate this process as much as possible. As you know this won't work for all commits, but there will be a subset of commits which do apply cleanly.

Using your manual mode we'd love to be able to script some version the following which runs when a new commit gets merged to OpenZFS.

Cherry pick the commit hash on to a new branch
Iff it applies cleanly run the style and compile check
Iff the checks pass open a new PR for automated testing

Since you've already implemented the majority of this I think the only missing bit is the ability to open a new PR. The good news here is that there's already an existing utility provided by GitHub which does this called hub. It's a wrapper which extends to git command to support things like opening PRs from the command line. If you can add this functionality to the script we should have everything we need.

Thanks for working on this!

gmelikov · 2017-01-26T18:02:01Z

@behlendorf thanks for straightforward usecase. I'll add this functionality little later, because while improving script i'm ready to check new commits by myself. But it's the best approach, i agree.

Main problems with auto merge:

different paths to files - maybe i'll add symlinks for main cases
little differences with line order (for ex. copyright comments) - the only decision is to make as identical as possible on manual merge. I already began this in OpenZFS 7254 - ztest failed assertion in ztest_dataset_dirobj_verify:… zfs#5670
after all we have a man pages problem (different formats)

behlendorf · 2017-01-26T18:23:22Z

different paths to files - maybe i'll add symlinks for main cases

This should be handled by git in the majority of cases. There are going to be a couple of files which it can't match because they've been either rewritten entirely for Linux (vdev_disk.c, zvol.c, zfs_ctldir.c) or currently only exist under Linux (zpl_.c, vdev_raidz_math.c). Have you noticed other files which can't me matched?

little differences with line order (for ex. copyright comments) - the only decision is to make as identical as possible on manual merge.

Agreed, that's the best we can do. I'm not opposed to applying patches to the Linux port which are just designed to minimize these tiny differences.

after all we have a man pages problem

Yes, we should grab the rewritten mdoc pages from Illumos as a starting point and then re-apply the Linux specific sections we've added. This would also be a good time to feed trivial fixes for spelling mistakes and such back upstream to OpenZFS.

dinatale2

Thanks for opening this pull request! Really excited about adding this to our set of scripts. Just a few comments looking at it. I'll be trying to use the script this afternoon and will post any other feedback.

dinatale2 · 2017-01-26T20:10:40Z

scripts/openzfs-merge.sh

+	-d directory	Git repo with openzfs and zfsonlinux remotes
+	-i commits.txt	File with OpenZFS commit hashes to merge
+	(one hash per row)
+	-i commit hash	Prepare branch and try to merge this commit by hash,


I think you meant -c here.

dinatale2 · 2017-01-26T20:11:42Z

scripts/openzfs-merge.sh

+	rm "$TMP_FILE"
+	git checkout master
+	git fetch --all
+	git log --remotes=openzfs/master --format=%B -n 1 $OPENZFS_COMMIT > "$TMP_FILE"


Is --remotes necessary here? The commit hash should be sufficient to find the log entry.

It's just for safety, we use short hashes.

dinatale2 · 2017-01-26T20:12:42Z

scripts/openzfs-merge.sh

+SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+
+REPOSITORY_PATH='.'
+TMP_FILE='/tmp/gittmpmessage.txt'


I think you are using a file because it is easier to work with? If so, then this fine.

Yes, it was just easier for me.

dinatale2 · 2017-01-26T20:15:33Z

scripts/openzfs-merge.sh

+	fi
+
+	echo -e "${LGREEN} Cstyle... ${NORMAL}"
+	if ! make cstyle; then


I'm not sure if you should fail if cstyle fails. The patch may work and would involve minor tweaking to address the style problems. Thoughts?

It may be appropriate to add a flag to the script to control this behavior.

I thought in this way: patch have to be edited -> fail, overwise success.
Yes, it's easy to add a flag, i'll add it.

dinatale2 · 2017-01-26T20:23:11Z

scripts/openzfs-tracking.sh

@@ -81,6 +81,8 @@ OPTIONS:
 	-h		Show this message
 	-d directory	Git repo with openzfs and zfsonlinux remotes
 	-e exceptions	Exception file (using ZoL wiki if not specified)
+	-c file.txt		Write OpenZFS unmerged commits' hashes to file,


Tabbing/Spacing needs to be fixed here.

dinatale2 · 2017-01-26T20:23:54Z

scripts/openzfs-tracking.sh

@@ -234,7 +239,7 @@ do

 	# Match issue against any open pull requests.
 	ZFSONLINUX_PR=$(echo $ZFSONLINUX_PRS | jq -r ".[] | select(.title | \
-	    contains(\"OpenZFS $OPENZFS_ISSUE\")) | { html_url: .html_url }" | \
+	    contains(\"OpenZFS $OPENZFS_ISSUE \")) | { html_url: .html_url }" | \


A rebase should get rid of this change.

dinatale2 · 2017-01-26T20:32:32Z

scripts/openzfs-merge.sh

+#
+# Repository setup for valid compilation check:
+#	sh autogen.sh
+#	./configure --enable-debug


Wouldn't it be safe to run sh autogen.sh and ./configure for each commit? Is it because Makefile.in's don't often change? There are other .in files as well.

It will we safer, but much more slower.

I didn't have any issues with it yet.

dinatale2 · 2017-01-26T20:40:50Z

scripts/openzfs-merge.sh

+
+clean_unmerged() {
+	git cherry-pick --abort
+	git checkout master


It may be worth documenting that this script should be used on a clone where the origin is a fork of ZFS on Linux that has a remote to OpenZFS.

There are already these lines:

# Repository setup must be similar with openzfs-tracking.sh # requirements.

openzfs-tracking.sh already has there requirements.

Ah, I missed that. Thanks.

dinatale2 · 2017-01-26T20:44:39Z

scripts/openzfs-merge.sh

+	git cherry-pick --abort
+	git checkout master
+	git branch -D "autoport-oz$OPENZFS_ISSUE"
+	rm "$TMP_FILE"


You may want to run a git clean -xdf. This would require you to rerun sh autogen.sh and ./configure.

Maybe, but this variant is faster and it won't remove untracked files.

dinatale2 · 2017-01-26T23:19:06Z

scripts/openzfs-merge.sh

+
+	echo -e "${LGREEN} - OpenZFS issue #$OPENZFS_ISSUE $OPENZFS_COMMIT ${NORMAL}"
+	echo -e "${LGREEN} Checkout new branch ${NORMAL}"
+	git branch -D "autoport-oz$OPENZFS_ISSUE"


You may want to change this to git branch -D "autoport-oz$OPENZFS_ISSUE" > /dev/null because it displays an error if the branch doesn't exist.

dinatale2 · 2017-01-27T00:58:23Z

scripts/openzfs-merge.sh

+	OPENZFS_COMMIT_AUTHOR=$(git log --format="%aN <%aE>" --remotes=openzfs/master -n 1 $OPENZFS_COMMIT)
+
+	sed -i '1s/^/OpenZFS /' "$TMP_FILE"
+	sed -i 's/ / - /2' "$TMP_FILE"


I believe this string replace intends to add a dash after the issue number, but it ends up putting dashes after each Reviewed by:. Also, it's not spaced correctly. Here's what I came up with:

generate_desc() { USER_NAME=$(git config user.name) USER_MAIL=$(git config user.email) OPENZFS_COMMIT_AUTHOR=$(git log --format="%aN <%aE>" --remotes=openzfs/master -n 1 $OPENZFS_COMMIT) OPENZFS_ISSUE=$(grep -oP '^[^0-9]*\K[0-9]+' -m 1 "$TMP_FILE") sed -i "1s/^$OPENZFS_ISSUE/OpenZFS $OPENZFS_ISSUE -/" "$TMP_FILE" sed -i "1 a Authored by: $OPENZFS_COMMIT_AUTHOR" "$TMP_FILE" sed -i '/^$/d' "$TMP_FILE" sed -i "/^OpenZFS $OPENZFS_ISSUE.*$/G" "$TMP_FILE" echo 'Ported-by: '$USER_NAME' <'$USER_MAIL'>' >> "$TMP_FILE" echo '' >> "$TMP_FILE" echo 'OpenZFS-issue: https://www.illumos.org/issues/'$OPENZFS_ISSUE >> "$TMP_FILE" echo 'OpenZFS-commit: https://github.com/openzfs/openzfs/commit/'$OPENZFS_COMMIT >> "$TMP_FILE" }

Thanks, fixed this problem.

gmelikov · 2017-01-27T11:33:28Z

@behlendorf just have examples for different paths:
commit openzfs/openzfs@39cddb1

	deleted by us:   usr/src/uts/common/fs/zfs/sys/vdev_disk.h
	deleted by us:   usr/src/uts/common/fs/zfs/vdev_disk.c
	deleted by us:   usr/src/uts/common/os/devcfg.c

I think that's because these files are reworked for Linux, so this commit may be unmergable.

behlendorf · 2017-01-27T17:28:41Z

So in this case the vdev_disk.* files have been effectively rewritten from scratch for Linux and devcfg.c isn't part of ZFS just illumos. I'll update the exceptions page with a comment describing the situation for this commit.

behlendorf · 2017-01-31T17:47:13Z

@gmelikov let's get this merged as an initial version of the script. Do you have any additional fixes yould like to make first?

gmelikov · 2017-01-31T18:28:18Z

@behlendorf i don't have any for now, so let's merge it. Thanks for review!

gmelikov force-pushed the automerge branch 3 times, most recently from 3da14fa to a4874a1 Compare January 25, 2017 18:13

gmelikov mentioned this pull request Jan 25, 2017

OpenZFS 7386 - zfs get does not work properly with bookmarks openzfs/zfs#5666

Merged

gmelikov force-pushed the automerge branch from a4874a1 to c8d9985 Compare January 25, 2017 19:55

This was referenced Jan 25, 2017

OpenZFS 6676 - Race between unique_insert() and unique_remove() cause… openzfs/zfs#5667

Merged

OpenZFS 7254 - ztest failed assertion in ztest_dataset_dirobj_verify:… openzfs/zfs#5670

Merged

dinatale2 reviewed Jan 26, 2017

View reviewed changes

dinatale2 reviewed Jan 27, 2017

View reviewed changes

gmelikov force-pushed the automerge branch from c8d9985 to 5596a12 Compare January 27, 2017 11:22

Add OpenZFS auto merge script

23a794a

gmelikov force-pushed the automerge branch from 5596a12 to 23a794a Compare January 27, 2017 11:27

dinatale2 merged commit 3145978 into openzfs:master Jan 31, 2017

dinatale2 mentioned this pull request Feb 3, 2017

Automate OpenZFS patch porting #50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenZFS auto merge script #51

Add OpenZFS auto merge script #51

gmelikov commented Jan 25, 2017 •

edited

Loading

gmelikov commented Jan 25, 2017 •

edited

Loading

behlendorf commented Jan 26, 2017

gmelikov commented Jan 26, 2017 •

edited

Loading

behlendorf commented Jan 26, 2017

dinatale2 left a comment

dinatale2 Jan 26, 2017

dinatale2 Jan 26, 2017

gmelikov Jan 27, 2017

dinatale2 Jan 26, 2017

gmelikov Jan 27, 2017

dinatale2 Jan 26, 2017

behlendorf Jan 26, 2017

gmelikov Jan 27, 2017 •

edited

Loading

dinatale2 Jan 26, 2017

dinatale2 Jan 26, 2017

dinatale2 Jan 26, 2017

gmelikov Jan 27, 2017

dinatale2 Jan 26, 2017

gmelikov Jan 27, 2017

dinatale2 Jan 27, 2017

dinatale2 Jan 26, 2017

gmelikov Jan 27, 2017

dinatale2 Jan 26, 2017

dinatale2 Jan 27, 2017

gmelikov Jan 27, 2017

gmelikov commented Jan 27, 2017 •

edited

Loading

behlendorf commented Jan 27, 2017

behlendorf commented Jan 31, 2017

gmelikov commented Jan 31, 2017

Add OpenZFS auto merge script #51

Add OpenZFS auto merge script #51

Conversation

gmelikov commented Jan 25, 2017 • edited Loading

gmelikov commented Jan 25, 2017 • edited Loading

behlendorf commented Jan 26, 2017

gmelikov commented Jan 26, 2017 • edited Loading

behlendorf commented Jan 26, 2017

dinatale2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmelikov Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmelikov commented Jan 27, 2017 • edited Loading

behlendorf commented Jan 27, 2017

behlendorf commented Jan 31, 2017

gmelikov commented Jan 31, 2017

gmelikov commented Jan 25, 2017 •

edited

Loading

gmelikov commented Jan 25, 2017 •

edited

Loading

gmelikov commented Jan 26, 2017 •

edited

Loading

gmelikov Jan 27, 2017 •

edited

Loading

gmelikov commented Jan 27, 2017 •

edited

Loading