Reviewing of the pull requests

Hi,

the reviewing of the pull requests is currently not as easy as we would like to have. For simple changes it should be enough to go to the pull requests list (https://github.com/pharo-project/pharo/pulls) and review the changes from the GitHub web interface. In case that the PR passes tests and bootstrapping, it should be fine.

However if you want to play with an image that contains the PR code, your options are more complicated. The basic and the most clean one is to bootstrap from the repository with the PR applied on it. But it takes A LOT of time (over 20 mins). You can use this script that you will run inside the clone.

set -e

export PR=122

export BRANCH=development

export PHARO_VERSION=60

export BOOTSTRAP_ARCH=32 

git fetch origin $BRANCH

git fetch origin refs/pull/$PR/head

git checkout -b pullrequest FETCH_HEAD

git merge --no-edit origin/$BRANCH

#git diff origin/$BRANCH..pullrequest > pullrequest.diff

wget -O - get.pharo.org/${PHARO_VERSION}+vm | bash

./pharo Pharo.image --no-default-preferences ./bootstrap/scripts/prepare_image.st --save --quit

./pharo Pharo.image --no-default-preferences ./bootstrap/scripts/bootstrap.st --ARCH=${BOOTSTRAP_ARCH} --quit

bash ./bootstrap/scripts/build.sh

The other option is to take existing Pharo 7 image, have pharo-project/pharo clone next to it in a folder named 'pharo-core' and then create a new branch with the merged pull request. Currently it cannot be done from Iceberg directly so you need to use Git. You can use this script:

set -e

PR=119

COMMIT=2225314a7404c9e00fe71e70d41fc59d4ba78e2d

cd pharo-core

git fetch origin refs/pull/$PR/head

git checkout -b pr$PR-local FETCH_HEAD

git checkout $COMMIT

git checkout -b pr$PR-merged

git merge --no-edit pr$PR-local

#git diff $COMMIT...pr$PR-merged > pr$PR.diff

Set the pull request name to the variable PR and the image commmit hash to the variable named COMMIT. It can be obtained by "SystemVersion current commitHash". As the result you are in a merged branch created for this pull requests (e.g. pr122-merged) and you need to reload all packages. It can be done from Iceberg where you will need to confirm several dialogs, or by this script:

repo := MCFileTreeRepository new

directory: './pharo-core/src' asFileReference;

yourself.

versions := OrderedCollection new.

repo allFileNames do: [ :packageName |

| wc |

wc := MCWorkingCopy allManagers detect: [ :each | each packageName = (packageName withoutSuffix: '.package') ] ifNone: [nil].

wc ifNotNil: [

[(repo loadVersionFromFileNamed: packageName) load] on: MCMergeOrLoadWarning do: [:w | w resume ] ]] displayingProgress: [:e | e ].

The other option is to let display the classical Monticello merging window. For that you can use this script:

repo := MCFileTreeRepository new

directory: './pharo-core/src' asFileReference;

yourself.

versions := OrderedCollection new.

repo allFileNames do: [ :packageName |

| wc |

wc := MCWorkingCopy allManagers detect: [ :each | each packageName = (packageName withoutSuffix: '.package') ] ifNone: [nil].

wc ifNotNil: [

versions add: (repo loadVersionFromFileNamed: packageName) ]] displayingProgress: [:e | e ].

merger := MCVersionMerger new.

versions do: [:each | merger addVersion: each  ].

merger mergeWithNameLike: 'merged'.

merger gatherChanges.

(merger instVarNamed: #records) do: [ :each | each mergePatch operations do: [:operation | operation chooseRemote.]].

(merger instVarNamed: #merger) operations do: #chooseRemote.

merger resolveConflicts.

However I have seen cases where for example the PR deleted some class and the MC merging tool was not able to see this change. So be careful with this approach. We need to look at that.

Do not look on this mail as something final. It's only a hint for brave sprinters. To find a good workflow how properly review our PR should be one of our priorities.

Cheers,

-- Pavel

On Fri, Jun 30, 2017 at 4:28 PM, Pavel Krivanek <[hidden email]> wrote:

Hi,

the reviewing of the pull requests is currently not as easy as we would like to have. For simple changes it should be enough to go to the pull requests list (https://github.com/pharo-project/pharo/pulls) and review the changes from the GitHub web interface. In case that the PR passes tests and bootstrapping, it should be fine.

However if you want to play with an image that contains the PR code, your options are more complicated. The basic and the most clean one is to bootstrap from the repository with the PR applied on it. But it takes A LOT of time (over 20 mins). You can use this script that you will run inside the clone.

set -e

export PR=122

export BRANCH=development

export PHARO_VERSION=60

export BOOTSTRAP_ARCH=32 

git fetch origin $BRANCH

git fetch origin refs/pull/$PR/head

git checkout -b pullrequest FETCH_HEAD

git merge --no-edit origin/$BRANCH

#git diff origin/$BRANCH..pullrequest > pullrequest.diff

wget -O - get.pharo.org/${PHARO_VERSION}+vm | bash

./pharo Pharo.image --no-default-preferences ./bootstrap/scripts/prepare_image.st --save --quit

./pharo Pharo.image --no-default-preferences ./bootstrap/scripts/bootstrap.st --ARCH=${BOOTSTRAP_ARCH} --quit

bash ./bootstrap/scripts/build.sh

The other option is to take existing Pharo 7 image, have pharo-project/pharo clone next to it in a folder named 'pharo-core' and then create a new branch with the merged pull request. Currently it cannot be done from Iceberg directly so you need to use Git. You can use this script:

set -e

PR=119

COMMIT=2225314a7404c9e00fe71e70d41fc59d4ba78e2d

where does this commit hash come from?

 

cd pharo-core

git fetch origin refs/pull/$PR/head

git checkout -b pr$PR-local FETCH_HEAD

git checkout $COMMIT

git checkout -b pr$PR-merged

git merge --no-edit pr$PR-local

#git diff $COMMIT...pr$PR-merged > pr$PR.diff

Sorry I haven't the time to test this, but maybe it would do the same...??

git fetch origin refs/pull/$PR/merge:pr$PR-merged

#git diff $COMMIT...pr$merged 

https://coderwall.com/p/z5rkga/github-checkout-a-pull-request-as-a-branch

cheers -ben

Set the pull request name to the variable PR and the image commmit hash to the variable named COMMIT. It can be obtained by "SystemVersion current commitHash". As the result you are in a merged branch created for this pull requests (e.g. pr122-merged) and you need to reload all packages. It can be done from Iceberg where you will need to confirm several dialogs, or by this script:

repo := MCFileTreeRepository new

directory: './pharo-core/src' asFileReference;

yourself.

versions := OrderedCollection new.

repo allFileNames do: [ :packageName |

| wc |

wc := MCWorkingCopy allManagers detect: [ :each | each packageName = (packageName withoutSuffix: '.package') ] ifNone: [nil].

wc ifNotNil: [

[(repo loadVersionFromFileNamed: packageName) load] on: MCMergeOrLoadWarning do: [:w | w resume ] ]] displayingProgress: [:e | e ].

The other option is to let display the classical Monticello merging window. For that you can use this script:

repo := MCFileTreeRepository new

directory: './pharo-core/src' asFileReference;

yourself.

versions := OrderedCollection new.

repo allFileNames do: [ :packageName |

| wc |

wc := MCWorkingCopy allManagers detect: [ :each | each packageName = (packageName withoutSuffix: '.package') ] ifNone: [nil].

wc ifNotNil: [

versions add: (repo loadVersionFromFileNamed: packageName) ]] displayingProgress: [:e | e ].

merger := MCVersionMerger new.

versions do: [:each | merger addVersion: each  ].

merger mergeWithNameLike: 'merged'.

merger gatherChanges.

(merger instVarNamed: #records) do: [ :each | each mergePatch operations do: [:operation | operation chooseRemote.]].

(merger instVarNamed: #merger) operations do: #chooseRemote.

merger resolveConflicts.

However I have seen cases where for example the PR deleted some class and the MC merging tool was not able to see this change. So be careful with this approach. We need to look at that.

Do not look on this mail as something final. It's only a hint for brave sprinters. To find a good workflow how properly review our PR should be one of our priorities.

Cheers,

-- Pavel