Changes between Version 20 and Version 21 of SubmittingPatches
- Timestamp:
- Jan 23, 2017, 1:07:08 PM (7 years ago)
Legend:
- Unmodified
- Added
- Removed
- Modified
-
SubmittingPatches
v20 v21 1 This document explains the process to submit code patches. This process isn't well suited for art changes, it's best to use the forums to submit new proposed art.1 We use [wiki:Phabricator] to handle all code contributions, both from contributors and team members. This platform allows us to perform reviews in an efficient way, and it also makes automated testing of patches easy. This guide gives you details about our workflow, please read it. 2 2 3 == Creating the patch == 3 At any point in the process, it is advisable to drop on the [http://webchat.quakenet.org/?channels=0ad-dev IRC channel] to discuss your code or just to chat with us. 4 5 As of today, we still use Trac as a wiki and bugtracker while Phabricator is only used to manage the code. 6 7 NB: This process isn't well suited for art changes, it's best to use the forums to submit new proposed art. 8 9 10 == Making some changes == 4 11 First you should [[BuildInstructions|check out]] the game code from SVN or an up-to-date Git fork and make some changes. 12 13 Ideally, those changes are already described in one of our Trac tickets, so you know it is wanted. It would be a shame if you put some effort in a code that we actually don't want. If your change is a feature, it needs to have a ticket associated so we can discuss whether the feature is wanted and how. If it is a bugfix or a tiny change in the code, it's not mandatory to create a new ticket. 5 14 6 15 Make sure you're on the latest version of the code (and merged in your own changes) when making your patch. And try to follow the [[Coding_Conventions|coding conventions]]. … … 8 17 If this is your first contribution, please add yourself to the relevant file under [source:ps/trunk/binaries/data/mods/public/gui/credits/texts]. You are free to use your real name, your nickname or both. 9 18 10 Go to the "trunk" directory in the source tree, and create a patch, with `svn diff` (or the equivalent "create patch" feature in TortoiseSVN). 11 12 Also try to keep your patches reasonable. Different bugs shouldn't be mixed in the same patch. 19 Always try to keep your patches reasonable. Different bugs shouldn't be mixed in the same patch. 13 20 14 21 When you modify or add methods to simulation components, you should add unit tests that test all added or modified changes. 15 22 16 == The [report:21 RFC queue]==23 === Legal disclaimer === 17 24 18 Submitting a patch is done by attaching the patch file to a Trac ticket. This can be an existing relevant ticket or a new ticket.25 By sending in a patch, you agree that it is your own work (or else make it clear where it came from) and agree to licensing any code as [http://www.opensource.org/licenses/GPL-2.0 GPL 2+] (or in some cases [http://www.opensource.org/licenses/mit-license.php MIT], especially for code in `lib/` - check the existing copyright headers on the source files you edit) and data files as [http://creativecommons.org/licenses/by-sa/3.0/ CC-BY-SA 3.0]. 19 26 20 RFC stands for "request for comments".21 To get the ticket in the [report:21 RFC queue], you should make sure the ticket looks as follows:22 27 23 * The ''Summary'' (title of the ticket) should start with `[PATCH]`. 24 * The ''Milestone'' should be set to 'Work In Progress' if it is not set to current Alpha. Otherwise, it should remain in the current Alpha. 25 * The ''Keyword list'' should have the keywords `rfc patch`. 28 == Creating a revision on Phabricator for review == 26 29 27 At this stage, at least one other developer should try to give some comments on the patch (most importantly whether all requirements have been identified, whether the code design is going in the right direction and how the code should be shaped to stay maintainable).30 You need to create a revision containing a patch and relevant information on Phabricator. Read [wiki:Phabricator#UsingDifferential] for the technical details. 28 31 29 It is advisable to drop by on [https://kiwiirc.com/client/irc.quakenet.org#0ad-dev irc] to discuss matters. If the topic involves controversial matter (things that influence balancing or the game design in unforseen ways), it is recommended to create a forum topic for it. 32 Don't forget to put a reference to the Trac ticket when creating a revision on Phabricator (using the "Trac Tickets" field). 33 34 === Update the associated Trac ticket (if relevant) === 35 36 Notify the followers of the Trac ticket by posting an update on it. Put a link to the Differential revision and change the Milestone from Backlog to Work In Progress. 37 38 The Alpha milestones are meant to plan our next version, so don't touch them. When the ticket is closed as fixed, we will change the Milestone from Work In Progress to the upcoming Alpha. 39 40 == Patch review == 41 42 Upon uploading the patch, an automated build, along with a number of tests, will be run on your change. Wait for them to complete and, if it fails, try to address the issue. If it is successful, wait a bit for the patch to get a reviewer assigned to it. If it doesn't in the following few days, don't hesitate to ping us over IRC. 30 43 31 44 Some patches can take longer to review due to high complexity, lower priority or other circumstances. But it's best to keep in touch with us and ask for reviews. 32 45 33 Note that non-team members can help in the RFC process too by playtesting, checking code quality or even just seeing if it compiles on your system.46 The reviewer will test your patch and give you comments. When it is ready, it should be committed. However it is not the end of the journey! If we discover later that your patch had a hidden flaw, we will ask you to work on a fix. Usually, the reviewer who accepted your patch will contact you. 34 47 35 Wh en you're already familiar with the codebase and have a patch that won't cause discussions on the code style or implementation of that feature, you can skip the RFC stage, and push the patch to the review queue directly.48 While your patch awaits for review, and if you are interested in contributing, you are encouraged to review other people's patches! We have review guidelines at [wiki:ReviewingPatches], and you can also just test the patches. 36 49 37 == The [report:15 review queue]==50 == What changed with the new workflow? == 38 51 39 Once a patch is agreed on during the RFC process, it can be pushed to the [report:15 review queue]. This is done by changing the `rfc` keyword to `review`.52 The patch submission workflow recently changed after we set up [wiki:Phabricator] for code reviews. 40 53 41 At this point, one of the core developers should review the final patch on the following points: 54 The patches currently on Trac should be re-submitted by their authors following the guidelines above. 42 55 43 * Code style (check if the patch follows all [[Coding_Conventions|Coding Conventions]]) 44 * An in-game test (check if the patch implements the advertised changes) 45 * Unit tests (check if the required tests are implemented and the tests still work) 46 * Completeness (verifying that there are no files forgotten which require similar changes) 56 We don't need anymore the [PATCH] tag or the patch, review, rfc keywords, since no code contribution should be reviewed on Trac. Remove them when you port a patch to Phabricator, but there is '''no need to batch modify old tickets.''' 47 57 48 A code quality tool like jshint is recommended to test for non-obvious syntax mistakes. 49 50 When the patch passes on all points, it gets committed. When the patch doesn't pass, it will be sent back to the RFC queue. 51 52 == Legal disclaimer == 53 54 By sending in a patch, you agree that it is your own work (or else make it clear where it came from) and agree to licensing any code as [http://www.opensource.org/licenses/GPL-2.0 GPL 2+] (or in some cases [http://www.opensource.org/licenses/mit-license.php MIT], especially for code in `lib/` - check the existing copyright headers on the source files you edit) and data files as [http://creativecommons.org/licenses/by-sa/3.0/ CC-BY-SA 3.0]. 58 '''We would also like to preserve the current review/rfc queues so we know what still needs to be ported to Phabricator.''' They will be deleted when they are "naturally" empty.
