Spigot Pull Request: HOW-TO

Discussion in 'Spigot Discussion' started by Aikar, Feb 26, 2013.

  1. I'm sure many of you have been wondering how to do pull requests to Spigot ever since Spigot switched to the new patch system.

    Well since I pretty much designed the system, I figured I would write a guide on how to do it.

    There's 2 ways to do this however

    If your change is small or Is adding a new patch, and does not have Spigot-API dependency:

    • Checkout Spigot-Server or Spigot-API repo for the change you need to make
    • Make your change just as you would otherwise.
    • Push to your fork / PR
    • Inform md_5, and he can pull the commit in just as the old style was.
    However, if your commit is larger, and is going to be editing a previous commit, you should use the patch system.
    WARNING: This only works on Mac and Linux environments. Windows may be able to do this from CYGWIN but untested.

    In this guide,
    I will say Patch Repo to represent github.com/EcoCityCraft/Spigot - This is the root repo you are working in.

    I will say Spigot repo to represent github.com/EcoCityCraft/Spig0t-Server, which is the Spigot-Server folder inside of the Patch repo

    I will say API repo to represent github.com/EcoCityCraft/Spigot-API, which is the Spigot-API folder inside of the Patch repo.
    • checkout the Patch repo
    • create a branch for your change and check it out (git checkout -b mybranchname)
    • run ./applyPatches.sh

      This command will take the current patch files, and build the API and Server repos off of the patch files.

      This will ensure your current repos are synced to the state of your Patch repo checkout.
    • type mvn clean install to do a priming build and install both jars to your local maven.
    • cd into Spigot-API or Spigot-Server, where-ever you need to make your change.
    • At this point, you are free to use git rebase to edit, delete, rename or rearrange commits
      When you are done you will be making a "snapshot", so rebase is perfectly safe to use!

      However, stay on master repo checkout! I have not verified what will happen if you use a different branch yet.
    • Make any changes you need to Spigot-API and Spigot-Server. If you are modifying code that belongs to another patch, make the changes using git rebase and the edit opcode, or squash your commit into the other one using rebase.

      Do not add new patches for changes to an existing patch! New patches should be made for new work.
    • Once you are done, cd one level up back to your root Patch checkout.
    • type mvn clean install to compile both.
    • TEST! your spigot build will be in <Patch repo>/Spigot-Server/target/
    • If your changes are good, and commited to the proper state, move on to next, otherwise go back and fix things.
    • in the Patch repo, issue
      Code (Text):
      rm -rf *-Patches/*.patch
      (hopefully this will not be required in the future, but md_5 needs to change his workflow a little to support automating this step)

      This will clean up the old patches to prepare for the new patches. Skipping this step will potentially end up with bad things happening such as old patches surviving a rename and things will be broken badly.

      ALWAYS CLEAN UP FIRST!
    • After the patches are clean, issue ./rebuildPatches.sh

      This will take the current state of the API and Server repos and create the patch files to snapshot to the exact state you have them at.
    • When you now issue git status, you should see changes to patch files related to your change.

      If you added a new patch, there should only be a single new file addition

      If you modified an existing, nothing before that patch should be modified. However, patches AFTER the one you modified may be modified. This will be because A) You solved conflicts, B) git metadata updates that is required for clean application.

      Metadata changes simply look like this:
      Code (Diff):
      diff --git a/CraftBukkit-Patches/0030-Kick-player-on-exception-for-built-in-PluginChannels.patch b/CraftBukkit-Patches/0030-Kick-player-on-exception-for-built-in-PluginChannels.patch
      index 653ade8..1db74c8 100644
      --- a/CraftBukkit-Patches/0030-Kick-player-on-exception-for-built-in-PluginChannels.patch
      +++ b/CraftBukkit-Patches/0030-Kick-player-on-exception-for-built-in-PluginChannels.patch
      @@ -1,4 +1,4 @@
      -From 90a010f980ef943857f4437f64e76761ad6c37d3 Mon Sep 17 00:00:00 2001
      +From f79f9c8a4b8dea443b5477c742d76f4ca8a780bb Mon Sep 17 00:00:00 2001
      From: Eimref <[EMAIL][EMAIL][EMAIL][EMAIL][email protected][/EMAIL][/EMAIL][/EMAIL][/EMAIL]>
      Date: Wed, 6 Feb 2013 18:59:07 -0500
      Subject: [PATCH] Kick player on exception for built-in PluginChannels; Fixes
      @@ -9,10 +9,10 @@ Subject: [PATCH] Kick player on exception for built-in PluginChannels; Fixes
        1 file changed, 8 insertions(+), 3 deletions(-)

      diff --git a/src/main/java/net/minecraft/server/PlayerConnection.java b/src/main/java/net/minecraft/server/PlayerConnection.java
      -index 7ca0acf..d1e0207 100644
      +index d2c2305..b960a87 100644
      --- a/src/main/java/net/minecraft/server/PlayerConnection.java
      +++ b/src/main/java/net/minecraft/server/PlayerConnection.java
      [email protected]@ -1483,6 +1483,7 @@ public class PlayerConnection extends Connection {
      [email protected]@ -1481,6 +1481,7 @@ public class PlayerConnection extends Connection {

      --
      -1.8.1-rc2
      +1.8.1.1
      these are fine and need to be commited.

      Deleting lines sometimes causes other lines to popup in these too, dont worry, its generally ok if you see changes popping up in commits you did not touch.
    • You may now commit your patch changes. DO NOT add Spigot-Server/Spigot-API folders to git (do not use git add .) I am unsure why they are not on .gitignore, but they are not meant to be commited.

      In your commit describe what changed. If your editing a previous patch, describe what you changed about that patch (do not need to redescribe the entire patch), but if its a new patch, describe what it does.
    • You may now git push origin branchname to push it to your forks branch.
    • Login to github and issue the pull request.
    Now... If your change is big, it may be difficult to to review your changes, however you can pretty easily generate a change diff of your entire change to supply with your PR like I did here:
    https://github.com/EcoCityCraft/Spigot/pull/16#commits-pushed-b00acdd

    It's a little bit tedius, but it helps md_5 and others a lot in reviewing your changes.
    • If your not on your Patch Repo branch, do so (git checkout featurebranch)
    • If your API and Spigot repos are not still on your feature changes, then type ./applyPatches.sh
    • You should now be back to the state of when you made your changes
    • Inside of the API and Server repos, Create a branch off of master with your features changes (but don't check it out): git branch feature.

      Do this for both API and Spigot that you have changes in.
    • You should still be on master in API/Server, now cd .. back into your Patch repo.
    • checkout the master repo of your Patch repo (git checkout master)
    • ./applyPatches.sh
    • Your Server and API repos should now be at master WITHOUT your changes (We saved your changes to the branches in each)
    • cd into Spigot-Server (and repeat this for API if needed too), and git checkout myfeature
    • git log should show your changes still
    • type git reset --soft master

      It is important to remember the --soft and not --hard...
    • you should now see all of your changed files on git status.
    • Now simply git commit -a to commit all of your changes, and do a message like "PR ## Changes".
    • This builds a single commit with every thing you changed.
    • If you maintain your Repo checkout, you only need to do this once
      you will need a fork of API and Spigot-Server repos too.
      Add a new remote that points to your github account like so:

      Code (Text):
      git remote add myuser github.com:myuser/Spigot-Server
      But use Spigot-API for the API repo of course.

      Now git fetch myuser to pull in your forks Server/API data.
    • now you may push this branch to your fork with git push myuser feature -f
    • Login to github and you should see this commit in the branch on your Spigot-Server/Spigot-API repo, and simply supply a link to the commits on your Pull Request for easy diff viewing.
    Yes, this process is alot more complicated, but allows alot more flexability, reliability, changelogs and ability to restore older state alot easier.

    I will adapt this guide with any feedback I can get. I'm not the best at writing guides... so tell me how to make it easier to understand if you can!
     
    #1 Aikar, Feb 26, 2013
    Last edited: Mar 13, 2013
    • Like Like x 2
    • Informative Informative x 2
    • Winner Winner x 1
  2. md_5 has renamed Spigot folder to be Spigot-Server for the patched CB dir (so its alot less confusing now)

    So I have updated the guide with appropriate naming.
     
  3. My head hurts.