CONTRIBUTING.md

   1 # ISC DHCP Contributor's Guide
   2
   3 So you found a bug in ISC DHCP or plan to develop an extension and want to send us a patch? Great!
   4 This page will explain how to contribute your changes smoothly.
   5
   6 ## Writing a patch
   7
   8 Before you start working on a patch or a new feature, it is a good idea to discuss it first with
   9 DHCP developers. You can post your questions to the [dhcp-workers](https://lists.isc.org/mailman/listinfo/dhcp-workers)
  10 or [dhcp-users](https://lists.isc.org/mailman/listinfo/dhcp-users) mailing lists. The dhcp-users is
  11 intended for users who are not interested in the internal workings or development details: it is
  12 OK to ask for feedback regarding new design or the best proposed solution to a certain problem.
  13 This is the best place to get user's feedback. The internal details, questions about the code and
  14 its internals are better asked on dhcp-workers. The dhcp-workers is a very low traffic list.
  15
  16 OK, so you have written a patch? Great! Before you submit it, make sure that your code compiles.
  17 This may seem obvious, but there's more to it. You have surely checked that it compiles on your
  18 system, but ISC DHCP is a portable software. Besides Linux, it is compiled and used on relatively
  19 uncommon systems like OpenBSD. Will your code compile and work there? What about endianness? It is
  20 likely that you used a regular x86 architecture machine to write your patch, but the software is
  21 expected to run on many other architectures. For a complete list of systems we build on, you may
  22 take a look at the [Jenkins build farm report](https://jenkins.isc.org/view/isc-dhcp/).
  23
  24 ## Running unit-tests
  25
  26 One of the ground rules in all ISC projects is that every piece of code has to be tested. For newer
  27 projects, such as Kea, we require unit-test for almost every line of code. For older code, such as
  28 ISC DHCP, that was not developed with testability in mind, it's unfortunately impractical to require
  29 extensive unit-tests. Having said that, please think thoroughly if there is any way to develop
  30 unit-tests. The long term goal is to improve the situation.
  31
  32 You should have also conducted some sort of system testing to verify that your changes do what you
  33 want. It would be extremely helpful if you can attach any configuration files (dhcpd and or
  34 dhclient), along with a step-by-step procedure to carry out the test(s).  This will help us verify
  35 your changes as extend our own system tests.
  36
  37 Building ISC DHCP code from the repository is slightly different than the release tarballs. One
  38 major difference is that it does not have BIND source bundled inside and those have to be
  39 downloaded separately. Fortunately, there's an easy to use script for that:
  40
  41 ```bash
  42 sh util/bind.sh v4_4
  43 ./configure --with-atf
  44 make
  45 ```
  46
  47 Make sure you have ATF (Automated Test Framework) installed in your system. For more information
  48 about ATF, please refer to <dhcp source tree>/doc/devel/atf.dox.  Note, running "make devel" in this
  49 directory will generate the documentation. To run the unit-tests, simply run:
  50
  51 ```bash
  52 make check
  53 ```
  54
  55 If you happen to add new files or have modified any Makefile.am files, it is also a good idea to
  56 check if you haven't broken the distribution process:
  57
  58 ```bash
  59 make distcheck
  60 ```
  61
  62 There are other useful switches which can be passed to configure. A complete list of all switches
  63 can be obtained with the command:
  64
  65 ```bash
  66 ./configure --help
  67 ```
  68
  69 ## Create an issue
  70
  71 Since you want to change something in ISC DHCP, there's a problem, deficiency or a missing feature.
  72 Quite often it is not clear why specific change is being made. The best way to explain it is to
  73 [create an issue here](https://gitlab.isc.org/isc-projects/dhcp/issues/new). We prefer the original
  74 submitter fill them as he or she has the best understanding of the purpose of the change and may
  75 have any extra information, e.g. "this patch fixes compilation issue on FreeBSD 10.1". If there there
  76 is no MR and no gitlab issue, we will create one. Depending on the subjective importance and urgency
  77 as perceived by the ISC engineer, the issue and/or MR will be assigned to one of the milestones.
  78
  79 ## Merge Request (also known as sending your patch the right way)
  80
  81 The first step in writing the patch or new feature should be to get the source code from our Git
  82 repository. The procedure is very easy and is [explained here](https://gitlab.isc.org/isc-projects/dhcp/wikis/gitlab-howto).
  83 While it is possible to provide a patch against the latest stable release, it makes the review
  84 process much easier if it is for latest code from the Git master branch.
  85
  86 Since you won't get write access to the ISC DHCP repository, you should fork it and then commit
  87 your changes to your own repo. How you organize the work depends entirely on you, but it seems
  88 reasonable to create a branch rather than working on your master.  Once you feel that your patch
  89 is ready, please commit your changes and push it to your copy of ISC DHCP repo. Then go to DHCP project
  90 and [submit a Merge Request](https://gitlab.isc.org/isc-projects/dhcp/merge_requests/new).
  91
  92 TODO: I don't think this is necessary. If you can't access this link or don't see New Merge Request
  93 button on the [merge requests page](https://gitlab.isc.org/isc-projects/dhcp/merge_requests)
  94 or the link gives you 404 error, please ask on dhcp-users and someone will help you out.
  95
  96 Once you submit it, someone from the DHCP development team will look at it and will get back to you.
  97 The dev team is very small, so it may take a while...
  98
  99 ## If you really can't do MR on gitlab...
 100
 101 Well, you are out of luck. There are other ways, but those are really awkward and the chances of
 102 your patch being ignored are really high. Anyway, here they are:
 103
 104 - Create a ticket in the DHCP Gitlab (https://gitlab.isc.org/isc-projects/dhcp) and attach your
 105   patch to it. Sending a patch has a number of disadvantages. First, if you don't specify the base
 106   version against which it was created, one of ISC engineers will have to guess that or go through
 107   a series of trials and errors to find that out. If the code doesn't compile, the reviewer will not
 108   know if the patch is broken or maybe it was applied to incorrect base code. Another frequent
 109   problem is that it may be possible that the patch didn't include any new files you have added.
 110
 111 - Send a patch to the dhcp-workers list. This is even worse, but still better than not getting the
 112   patch at all. The problem with this approach is that we don't know which version the patch was
 113   created against and there is no way to track it. So the chances of it being forgotten are high.
 114   Once a DHCP developer get to it, the first thing he/she will have to do is try to apply your
 115   patch, create a branch commit your changes and then open MR for it.
 116
 117 ## Going through a review
 118
 119 Once the MR is in the system, the action is on one of the ISC (and possibly other trusted) engineers.
 120
 121 Sooner or later, one of ISC engineers will do the review. Here's the tricky part. One of ISC DHCP
 122 developers will review your patch, but it may not happen immediately. Unfortunately, developers
 123 are usually working under a tight schedule, so any extra unplanned review work may take a while
 124 sometimes. Having said that, we value external contributions very much and will do whatever we
 125 can to review patches in a timely manner. Don't get discouraged if your patch is not accepted
 126 after first review. To keep the code quality high, we use the same review processes for external
 127 patches as we do for internal code. It may take some cycles of review/updated patch submissions
 128 before the code is finally accepted. The nature of the review process is that it emphasizes areas
 129 that need improvement. If you are not used to the review process, you may get the impression that
 130 the feedback is negative. It is not: even the ISC developers seldom see reviews that say "All OK
 131 please merge".
 132
 133 If we happen to have any comments that you as submitter are expected to address (and in the
 134 overwhelming majority of cases, we have), you will be asked to update your MR. It is not
 135 uncommon to see several rounds of such reviews, so this can get very complicated very quickly.
 136
 137 Once the process is almost complete, the developer will likely ask you how you would like to be
 138 credited. The typical answers are by first and last name, by nickname, by company name or
 139 anonymously. Typically we will add a note to the ChangeLog and also set you as the author of the
 140 commit applying the patch and update the contributors section in the AUTHORS file. If the
 141 contributed feature is big or critical for whatever reason, it may also be mentioned in release
 142 notes.
 143
 144 Sadly, we sometimes see patches that are submitted and then the submitter never responds to our
 145 comments or requests for an updated patch. Depending on the nature of the patch, we may either fix
 146 the outstanding issues on our own and get another ISC engineer to review them or the ticket may end
 147 up in our Outstanding milestone. When a new release is started, we go through the tickets in
 148 Outstanding, select a small number of them and move them to whatever the current milestone is. Keep
 149 that in mind if you plan to submit a patch and forget about it. We may accept it eventually, but
 150 it's much, much faster process if you participate in it.
 151
 152 ## Extra steps
 153
 154 If you are interested in knowing the results of more in-depth testing, you are welcome to visit the
 155 ISC Jenkins page: https://jenkins.isc.org This is a live result page with all tests being run on
 156 various systems. Besides basic unit-tests, we also have reports from valgrind (memory debugger),
 157 cppcheck and clang-analyzer (static code analyzers), Lettuce system tests and more. Although it
 158 is not possible for non ISC employees to run tests on that farm, it is possible that your
 159 contributed patch will end up there sooner or later. We also have ISC Forge tests running and other
 160 additional tests, but currently those test results are not publicly available.