Evaluating a Pull Request

This page mostly targets the Longturn Admins who will be asked to review (evaluate) a submitted Pull Request. It is the policy of the Longturn community that all (100%) Pull Requests have at least one reviewer complete an evaluation of a change and either approve or make suggestions for improvements before a merge into the master branch.

Code PRs

For each Pull Request, a set of tests are run automatically. When the tests do not pass, the author is expected to fix the problems before someone tries to test the code.

This page assumes the user knows how to use git, compile Freeciv21 and use GitHub.

Create A Testing Branch

$ git fetch upstream master
$ git checkout -b testing/pr_[pr-number] upstream/master

Get A Copy Of The Pull Request

$ wget https://github.com/longturn/freeciv21/pull/[pr-number].diff -O pr[pr-number].diff

Apply The Downloaded Update To Local

$ git apply pr[pr-number].diff

Ensure The Build Directory Is Empty

$ rm -Rf build_[pr-number]

Configure And Compile The Code

$ cmake . -B build_[pr-number] -G Ninja -DCMAKE_INSTALL_PREFIX=$PWD/build_[pr-number]/install
$ cmake --build build_[pr-number]
$ cmake --build build_[pr-number] --target install
$ cmake --build build_[pr-number] --target package      # MSYS2 and Debian Linux Only

Run tests

$ cmake --build build_[pr-number] --target test

Read The Issue’s Notes

Ensure you understand what is being reported. Test the scenario as written. Make notes/comments in the PR. Approve the review request if warranted. if not, state why. If further commits are added to the PR, you will probably have to re-download the diff and run another test.

Run an autogame

If it is a big change, it might be worthwhile to run an entire game with just AI to make sure it does not break anything. You can compile the code, with additional checks such as address sanitizer with $ cmake . --preset ASan. Once the code is compiled, you can run the autogame with $ ./build_[pr-number]/freeciv21-server -r ./data/test-autogame.serv. You can also observe the game with $ ./build_[pr-number]/freeciv21-client -a -p 5556 -s localhost. ASan by default halts on every error, this is sometimes useful to developers to fix the errors sequentially. If you’d rather prefer listing all the errors at once, set the environment variable using $ export ASAN_OPTIONS="halt_on_error=0"

Cleanup

  • Remove the downloaded diff files: $ rm *.diff.

  • Remove any untracked files: $ git status. Look for any untracked files and delete them

  • Stash changes: $ git stash.

  • Checkout the master branch and delete the testing branch:

$ git checkout master
$ git branch -d testing/pr_[pr-number]

Art PRs

If a Pull Request includes art (graphics, music, etc), you should check not only the inner quality of the art, but also how it fits within what is already there. It is sometimes preferable to use lower quality sprites if they fit better with the general style of a tileset.

A recurring issue with graphics and sound assets is their licensing and attribution. Much more than code, images and music files get copied over, merged, or renamed, and authorship information is quickly lost. Make sure that the author of the PR understands where the files come from and who authored them. If possible, ask the original author directly if we can include their art.

We request that all asset files be accompanied with license and copyright information in the form of a license file. You will find many examples in the repository. The license should be compatible with version 3 of the GPL.

Warning

Please be extra careful when submitted graphics are present on Freeciv-Web, as doubts have been repeatedly raised about the validity of some of the copyright claims made by the main developer of that project. We were also asked not to use their graphics, and we will respect this even if it would be allowed by law. As a rule, we only accept assets present on FCW if we can prove that they were taken from somewhere else — and in that case, we refer to the original source for licensing information.