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
