]>
Commit | Line | Data |
---|---|---|
b0ff7312 QY |
1 | Developing for FRRouting |
2 | ========================= | |
3 | ||
4 | ## Table of Contents | |
f6ee5b52 | 5 | |
4765f35e | 6 | [TOC] |
f6ee5b52 | 7 | |
02fe6f86 DL |
8 | ## General note on this document |
9 | ||
10 | This document is "descriptive/post-factual" in that it documents pratices that | |
11 | are in use; it is not "definitive/pre-factual" in prescribing practices. | |
12 | ||
13 | This means that when a procedure changes, it is agreed upon, then put into | |
14 | practice, and then documented here. If this document doesn't match reality, | |
15 | it's the document that needs to be updated, not reality. | |
16 | ||
17 | ||
f6ee5b52 DL |
18 | ## Git Structure |
19 | ||
c545559d | 20 | The master Git for FRRouting resides on Github at |
b0ff7312 | 21 | [https://github.com/frrouting/frr](https://github.com/FRRouting/frr) |
f6ee5b52 | 22 | |
9b7939d9 DL |
23 | ![git branches continually merging to the left from 3 lanes; float-right](doc/git_branches.svg |
24 | "git branch mechanics") | |
25 | ||
f1423462 QY |
26 | There is one main branch for development and a release branch for each major |
27 | release. | |
f6ee5b52 | 28 | |
02fe6f86 | 29 | New contributions are done against the head of the master branch. The CI |
f1423462 QY |
30 | systems will pick up the Github Pull Requests or the new patch from Patchwork, |
31 | run some basic build and functional tests. | |
f6ee5b52 | 32 | |
f1423462 QY |
33 | For each major release (1.0, 1.1 etc) a new release branch is created based on |
34 | the master. | |
f6ee5b52 | 35 | |
f1423462 QY |
36 | There was an attempt to use a "develop" branch automatically maintained by the |
37 | CI system. This is not currently in active use, though the system is | |
38 | operational. If the "develop" branch is in active use and this paragraph is | |
39 | still here, this document obviously wasn't updated. | |
02fe6f86 | 40 | |
f6ee5b52 DL |
41 | |
42 | ## Programming language, Tools and Libraries | |
43 | ||
c545559d QY |
44 | The core of FRRouting is written in C (gcc or clang supported) and makes use of |
45 | GNU compiler extensions. A few non-essential scripts are implemented in Perl | |
46 | and Python. FRRouting requires the following tools to build distribution | |
47 | packages: automake, autoconf, texinfo, libtool and gawk and various libraries | |
48 | (i.e. libpam and libjson-c). | |
f6ee5b52 DL |
49 | |
50 | If your contribution requires a new library or other tool, then please | |
c545559d QY |
51 | highlight this in your description of the change. Also make sure it’s supported |
52 | by all FRRouting platform OSes or provide a way to build without the library | |
53 | (potentially without the new feature) on the other platforms. | |
f6ee5b52 | 54 | |
f1423462 QY |
55 | Documentation should be written in Tex (.texi) or Markdown (.md) format with a |
56 | preference for Markdown. | |
f6ee5b52 DL |
57 | |
58 | ||
b0ff7312 QY |
59 | ## Mailing lists |
60 | ||
61 | Italicized lists are private. | |
62 | ||
63 | | Topic | List | | |
64 | |--------------------------------|------------------------------| | |
65 | | Development | dev@lists.frrouting.org | | |
66 | | Users & Operators | frog@lists.frrouting.org | | |
67 | | Announcements | announce@lists.frrouting.org | | |
68 | | _Security_ | security@lists.frrouting.org | | |
69 | | _Technical Steering Committee_ | tsc@lists.frrouting.org | | |
f6ee5b52 | 70 | |
f6ee5b52 DL |
71 | |
72 | ### Changelog | |
73 | ||
74 | The changelog will be the base for the release notes. A changelog entry for | |
75 | your changes is usually not required and will be added based on your commit | |
76 | messages by the maintainers. However, you are free to include an update to | |
77 | the changelog with some better description. The changelog will be the base | |
78 | for the release notes. | |
79 | ||
80 | ||
81 | ## Submitting Patches and Enhancements | |
82 | ||
b0ff7312 QY |
83 | ### Pre-submission Checklist |
84 | ||
85 | * Format code (see [Coding style requirements](#coding-style-requirements)) | |
86 | * Verify and acknowledge license (see [License for contributions](#license-for-contributions)) | |
87 | * Ensure you have properly signed off (see [Signing Off](#signing-off)) | |
88 | * Test building with various configurations: | |
89 | * `buildtest.sh` | |
90 | * Verify building source distribution: | |
91 | * `make dist` (and try rebuilding from the resulting tar file) | |
92 | * Run unit tests: | |
93 | * `make test` | |
94 | * Document Regression Runs and plans for continued maintenance of the feature | |
95 | ||
f6ee5b52 DL |
96 | ### License for contributions |
97 | ||
c545559d | 98 | FRRouting is under a “GPLv2 or later” license. Any code submitted must be |
f6ee5b52 DL |
99 | released under the same license (preferred) or any license which allows |
100 | redistribution under this GPLv2 license (eg MIT License). | |
101 | ||
b0ff7312 QY |
102 | ### Signing Off |
103 | ||
104 | Code submitted to FRRouting must be signed off. We have the same requirements | |
105 | for using the signed-off-by process as the Linux kernel. In short, you must | |
106 | include a signed-off-by tag in every patch. | |
f6ee5b52 | 107 | |
b0ff7312 QY |
108 | `Signed-off-by:` this is a developer's certification that he or she has the |
109 | right to submit the patch for inclusion into the project. It is an agreement to | |
110 | the Developer's Certificate of Origin (below). Code without a proper signoff | |
4b8ac525 | 111 | can not and will not be merged. |
b0ff7312 QY |
112 | |
113 | If you are unfamiliar with this process, you should read the [official policy | |
114 | at kernel.org](http://www.kernel.org/doc/Documentation/SubmittingPatches) and | |
115 | you might find this article about [participating in the Linux community on the | |
116 | Linux Foundation | |
117 | website](http://www.linuxfoundation.org/content/how-participate-linux-community-0) | |
118 | to be a helpful resource. | |
119 | ||
120 | In short, when you sign off on a commit, you assert your agreement to all of | |
121 | the following: | |
f6ee5b52 DL |
122 | |
123 | > Developer's Certificate of Origin 1.1 | |
124 | > | |
125 | > By making a contribution to this project, I certify that: | |
126 | > | |
127 | > (a) The contribution was created in whole or in part by me and I | |
128 | > have the right to submit it under the open source license | |
129 | > indicated in the file; or | |
130 | > | |
131 | > (b) The contribution is based upon previous work that, to the best | |
132 | > of my knowledge, is covered under an appropriate open source | |
133 | > license and I have the right under that license to submit that | |
134 | > work with modifications, whether created in whole or in part | |
135 | > by me, under the same open source license (unless I am | |
136 | > permitted to submit under a different license), as indicated | |
137 | > in the file; or | |
138 | > | |
139 | > (c) The contribution was provided directly to me by some other | |
140 | > person who certified (a), (b) or (c) and I have not modified | |
141 | > it. | |
142 | > | |
143 | > (d) I understand and agree that this project and the contribution | |
144 | > are public and that a record of the contribution (including all | |
145 | > personal information I submit with it, including my sign-off) is | |
146 | > maintained indefinitely and may be redistributed consistent with | |
147 | > this project or the open source license(s) involved. | |
148 | ||
b0ff7312 | 149 | ### What do I submit my changes against? |
e0ba80e2 DS |
150 | |
151 | We've documented where we would like to have the different fixes applied at | |
152 | https://github.com/FRRouting/frr/wiki/Where-Do-I-create-a-Pull-Request-against%3F | |
f1423462 QY |
153 | If you are unsure where your submission goes, look at that document or ask a |
154 | project maintainer. | |
f6ee5b52 | 155 | |
b0ff7312 | 156 | ### Github pull requests |
f6ee5b52 | 157 | |
b0ff7312 | 158 | The preferred method of submitting changes is a Github pull request. Code |
f1423462 QY |
159 | submitted by pull request will be automatically tested by one or more CI |
160 | systems. Once the automated tests succeed, other developers will review your | |
161 | code for quality and correctness. After any concerns are resolved, your code | |
162 | will be merged into the branch it was submitted against. | |
f6ee5b52 | 163 | |
b0ff7312 | 164 | ### Patch submission via mailing list |
f6ee5b52 | 165 | |
b0ff7312 QY |
166 | As an alternative submission method, a patch can be mailed to the development |
167 | mailing list. Patches received on the mailing list will be picked up by | |
168 | Patchwork and tested against the latest development branch. | |
f6ee5b52 DL |
169 | |
170 | The recommended way to send the patch (or series of NN patches) to the list is | |
b0ff7312 | 171 | by using `git send-email` as follows (assuming they are the N most recent |
f6ee5b52 DL |
172 | commit(s) in your git history: |
173 | ||
174 | ``` | |
b0ff7312 | 175 | git send-email -NN --annotate --to=dev@lists.frrouting.org |
f6ee5b52 DL |
176 | ``` |
177 | ||
178 | If your commits do not already contain a `Signed-off-by` line, then use the | |
b0ff7312 QY |
179 | following command to add it (after making sure you agree to the Developer |
180 | Certificate of Origin as outlined above): | |
f6ee5b52 DL |
181 | |
182 | ``` | |
b0ff7312 | 183 | git send-email -NN --annotate --signoff --to=dev@lists.frrouting.org |
f6ee5b52 DL |
184 | ``` |
185 | ||
b0ff7312 QY |
186 | Submitting multi-commit patches as a Github pull request is **strongly |
187 | encouraged** and increases the probability of your patch getting reviewed and | |
188 | merged in a timely manner. | |
f6ee5b52 DL |
189 | |
190 | ||
191 | ## After submitting your changes | |
192 | ||
193 | * Watch for Continuous Integration (CI) Test results | |
194 | * You should automatically receive an email with the test results within | |
195 | less than 2 hrs of the submission. If you don’t get the email, then check | |
196 | status on the github pull request (if submitted by pull request) or on | |
4765f35e | 197 | Patchwork at |
b0ff7312 | 198 | [https://patchwork.frrouting.org](https://patchwork.frrouting.org) (if |
4765f35e | 199 | submitted as patch to mailing list). |
4b8ac525 QY |
200 | * Please notify the development mailing list if you think something doesn’t |
201 | work. | |
f6ee5b52 DL |
202 | * If the tests failed: |
203 | * In general, expect the community to ignore the submission until the tests | |
204 | pass. | |
205 | * It is up to you to fix and resubmit. | |
b0ff7312 | 206 | * This includes fixing existing unit (“make test”) tests if your |
f6ee5b52 DL |
207 | changes broke or changed them. |
208 | * It also includes fixing distribution packages for the failing | |
4b8ac525 QY |
209 | platforms (ie if new libraries are required). |
210 | * Feel free to ask for help on the development list. | |
f6ee5b52 DL |
211 | * Go back to the submission process and repeat until the tests pass. |
212 | * If the tests pass: | |
b0ff7312 QY |
213 | * Wait for reviewers. Someone will review your code or be assigned to |
214 | review your code. | |
215 | * Respond to any comments or concerns the reviewer has. | |
216 | * After all comments and concerns are addressed, expect your patch to be | |
217 | merged. | |
f6ee5b52 DL |
218 | * Watch out for questions on the mailing list. At this time there will be a |
219 | manual code review and further (longer) tests by various community members. | |
b0ff7312 | 220 | * Your submission is done once it is merged to the master branch. |
f6ee5b52 DL |
221 | |
222 | ||
a03e3526 | 223 | ## Developer's Guidelines |
f6ee5b52 | 224 | |
c545559d | 225 | ### Source file header |
f6ee5b52 | 226 | |
4765f35e DL |
227 | New files need to have a Copyright header (see [License for |
228 | contributions](#license-for-contributions) above) added to the file. Preferred | |
229 | form of the header is as follows: | |
f6ee5b52 DL |
230 | |
231 | ``` | |
232 | /* | |
896014f4 DL |
233 | * Title/Function of file |
234 | * Copyright (C) YEAR Author’s Name | |
235 | * | |
236 | * This program is free software; you can redistribute it and/or modify it | |
237 | * under the terms of the GNU General Public License as published by the Free | |
238 | * Software Foundation; either version 2 of the License, or (at your option) | |
239 | * any later version. | |
240 | * | |
241 | * This program is distributed in the hope that it will be useful, but WITHOUT | |
242 | * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | |
243 | * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for | |
244 | * more details. | |
245 | * | |
246 | * You should have received a copy of the GNU General Public License along | |
247 | * with this program; see the file COPYING; if not, write to the Free Software | |
248 | * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA | |
f6ee5b52 | 249 | */ |
4765f35e DL |
250 | |
251 | #include <zebra.h> | |
f6ee5b52 DL |
252 | ``` |
253 | ||
c545559d | 254 | ### Adding copyright claims to existing files |
f6ee5b52 DL |
255 | |
256 | When adding copyright claims for modifications to an existing file, please | |
257 | preface the claim with "Portions: " on a line before it and indent the | |
258 | "Copyright ..." string. If such a case already exists, add your indented claim | |
259 | immediately after. E.g.: | |
260 | ||
261 | ``` | |
262 | Portions: | |
263 | Copyright (C) 2010 Entity A .... | |
264 | Copyright (C) 2016 Your name [optional brief change description] | |
265 | ``` | |
266 | ||
c545559d | 267 | ### Code formatting |
f6ee5b52 | 268 | |
b0ff7312 QY |
269 | FRR uses Linux kernel style except where noted below. Code which does not |
270 | comply with these style guidelines will not be accepted. | |
02fe6f86 | 271 | |
b0ff7312 | 272 | To assist with compliance, in the project root there is a .clang-format |
6058ea8c QY |
273 | configuration file which can be used with the `clang-format` tool from the LLVM |
274 | project. In the `tools/` directory there is a Python script named `indent.py` | |
275 | that wraps clang-format and handles some edge cases specific to FRR. If you are | |
276 | submitting a new file, it is recommended to run that script over the new file | |
277 | after ensuring that the latest stable release of `clang-format` is in your | |
278 | PATH. | |
02fe6f86 DL |
279 | |
280 | **Whitespace changes in untouched parts of the code are not acceptable in | |
281 | patches that change actual code.** To change/fix formatting issues, please | |
282 | create a separate patch that only does formatting changes and nothing else. | |
283 | ||
b0ff7312 | 284 | #### Style documentation |
6058ea8c | 285 | Kernel and BSD styles are documented externally: |
02fe6f86 | 286 | |
6058ea8c QY |
287 | * [https://www.kernel.org/doc/html/latest/process/coding-style.html](https://www.kernel.org/doc/html/latest/process/coding-style.html) |
288 | * [http://man.openbsd.org/style](http://man.openbsd.org/style) | |
02fe6f86 | 289 | |
6058ea8c | 290 | For GNU coding style, use `indent` with the following invocation: |
f6ee5b52 DL |
291 | |
292 | ``` | |
293 | indent -nut -nfc1 file_for_submission.c | |
294 | ``` | |
295 | ||
6058ea8c QY |
296 | #### Exceptions |
297 | ||
298 | FRR project code comes from a variety of sources, so there are some stylistic | |
c545559d | 299 | exceptions in place. They are organized here by branch. |
02fe6f86 | 300 | |
6058ea8c | 301 | **For `master`:** |
02fe6f86 | 302 | |
6058ea8c | 303 | BSD coding style applies to: |
02fe6f86 | 304 | |
c545559d | 305 | * `ldpd/` |
02fe6f86 | 306 | |
6058ea8c QY |
307 | `babeld` uses, approximately, the following style: |
308 | ||
309 | * K&R style braces | |
310 | * Indents are 4 spaces | |
311 | * Function return types are on their own line | |
312 | ||
313 | ||
314 | **For `stable/3.0` and `stable/2.0`:** | |
315 | ||
316 | GNU coding style apply to the following parts: | |
317 | ||
c545559d QY |
318 | * `lib/` |
319 | * `zebra/` | |
320 | * `bgpd/` | |
321 | * `ospfd/` | |
322 | * `ospf6d/` | |
323 | * `isisd/` | |
324 | * `ripd/` | |
325 | * `ripngd/` | |
326 | * `vtysh/` | |
02fe6f86 | 327 | |
6058ea8c | 328 | BSD coding style applies to: |
02fe6f86 | 329 | |
c545559d | 330 | * `ldpd/` |
02fe6f86 | 331 | |
02fe6f86 | 332 | |
a03e3526 QY |
333 | ### Documentation |
334 | ||
335 | FRRouting is a large and complex software project developed by many different | |
336 | people over a long period of time. Without adequate documentation, it can be | |
337 | exceedingly difficult to understand code segments, APIs and other interfaces. | |
338 | In the interest of keeping the project healthy and maintainable, you should | |
339 | make every effort to document your code so that other people can understand | |
340 | what it does without needing to closely read the code itself. | |
341 | ||
342 | Some specific guidelines that contributors should follow are: | |
343 | ||
344 | * Functions exposed in header files should have descriptive comments above | |
345 | their signatures in the header file. At a minimum, a function comment should | |
346 | contain information about the return value, parameters, and a general summary | |
347 | of the function's purpose. Documentation on parameter values can be omitted | |
348 | if it is (very) obvious what they are used for. | |
349 | ||
350 | Function comments must follow the style for multiline comments laid out in | |
351 | the kernel style guide. | |
352 | ||
353 | Example: | |
354 | ||
355 | ``` | |
356 | /* | |
357 | * Determines whether or not a string is cool. | |
358 | * | |
359 | * @param text - the string to check for coolness | |
360 | * @param is_clccfc - whether capslock is cruise control for cool | |
361 | * @return 7 if the text is cool, 0 otherwise | |
362 | */ | |
363 | int check_coolness(const char *text, bool is_clccfc); | |
364 | ``` | |
365 | ||
366 | The Javadoc-style annotations are not required, but you should still strive to | |
367 | make it equally clear what parameters and return values are used for. | |
368 | ||
369 | * Static functions should have descriptive comments in the same form as above | |
370 | if what they do is not immediately obvious. Use good engineering judgement | |
371 | when deciding whether a comment is necessary. If you are unsure, document | |
372 | your code. | |
373 | ||
374 | * Global variables, static or not, should have a comment describing their use. | |
375 | ||
376 | * **For new code in `lib/`, these guidelines are hard requirements.** | |
377 | ||
378 | ||
379 | If you are contributing code that adds significant user-visible functionality | |
380 | or introduces a new API, please document it in `doc/`. Markdown and LaTeX are | |
381 | acceptable formats, although Markdown is currently preferred for new | |
382 | documentation. This may change in the near future. | |
383 | ||
384 | Finally, if you come across some code that is undocumented and feel like going | |
385 | above and beyond, document it! We absolutely appreciate and accept patches that | |
386 | document previously undocumented code. | |
f6ee5b52 | 387 | |
c545559d | 388 | ### Compile-time conditional code |
f6ee5b52 | 389 | |
f1423462 QY |
390 | Many users access FRR via binary packages from 3rd party sources; compile-time |
391 | code puts inclusion/exclusion in the hands of the package maintainer. Please | |
392 | think very carefully before making code conditional at compile time, as it | |
393 | increases regression testing, maintenance burdens, and user confusion. In | |
394 | particular, please avoid gratuitous `--enable-…` switches to the configure | |
395 | script - in general, code should be of high quality and in working condition, | |
396 | or it shouldn’t be in FRR at all. | |
f6ee5b52 DL |
397 | |
398 | When code must be compile-time conditional, try have the compiler make it | |
c545559d QY |
399 | conditional rather than the C pre-processor so that it will still be checked by |
400 | the compiler, even if disabled. For example, | |
f6ee5b52 DL |
401 | |
402 | ``` | |
403 | if (SOME_SYMBOL) | |
404 | frobnicate(); | |
405 | ``` | |
406 | ||
c545559d | 407 | is preferred to |
f6ee5b52 DL |
408 | |
409 | ``` | |
410 | #ifdef SOME_SYMBOL | |
411 | frobnicate (); | |
412 | #endif /* SOME_SYMBOL */ | |
413 | ``` | |
414 | ||
415 | Note that the former approach requires ensuring that `SOME_SYMBOL` will be | |
416 | defined (watch your `AC_DEFINE`s). | |
7ce0cb3c | 417 | |
c545559d | 418 | ### Debug-guards in code |
7ce0cb3c | 419 | |
f1423462 QY |
420 | Debugging statements are an important methodology to allow developers to fix |
421 | issues found in the code after it has been released. The caveat here is that | |
422 | the developer must remember that people will be using the code at scale and in | |
423 | ways that can be unexpected for the original implementor. As such debugs | |
424 | **MUST** be guarded in such a way that they can be turned off. FRR has the | |
425 | ability to turn on/off debugs from the CLI and it is expected that the | |
426 | developer will use this convention to allow control of their debugs. | |
19c7f43f | 427 | |
c545559d | 428 | ### CLI changes |
19c7f43f | 429 | |
f1423462 QY |
430 | CLI's are a complicated ugly beast. Additions or changes to the CLI should use |
431 | a DEFUN to encapsulate one setting as much as is possible. Additionally as new | |
432 | DEFUN's are added to the system, documentation should be provided for the new | |
433 | commands. | |
e887b2b8 LB |
434 | |
435 | ### Backwards Compatibility | |
436 | ||
f1423462 QY |
437 | As a general principle, changes to CLI and code in the lib/ directory should be |
438 | made in a backwards compatible fashion. This means that changes that are purely | |
439 | stylistic in nature should be avoided, e.g., renaming an existing macro or | |
440 | library function name without any functional change. When adding new parameters | |
441 | to common functions, it is also good to consider if this too should be done in | |
442 | a backward compatible fashion, e.g., by preserving the old form in addition to | |
e887b2b8 LB |
443 | adding the new form. |
444 | ||
f1423462 QY |
445 | This is not to say that minor or even major functional changes to CLI and |
446 | common code should be avoided, but rather that the benefit gained from a change | |
447 | should be weighed against the added cost/complexity to existing code. Also, | |
448 | that when making such changes, it is good to preserve compatibility when | |
449 | possible to do so without introducing maintenance overhead/cost. It is also | |
450 | important to keep in mind, existing code includes code that may reside in | |
451 | private repositories (and is yet to be submitted) or code that has yet to be | |
452 | migrated from Quagga to FRR. | |
817302b8 DL |
453 | |
454 | That said, compatibility measures can (and should) be removed when either: | |
455 | ||
f1423462 QY |
456 | * they become a significant burden, e.g. when data structures change and the |
457 | compatibility measure would need a complex adaptation layer or becomes | |
817302b8 | 458 | flat-out impossible |
f1423462 QY |
459 | * some measure of time (dependent on the specific case) has passed, so that the |
460 | compatibility grace period is considered expired. | |
817302b8 DL |
461 | |
462 | In all cases, compatibility pieces should be marked with compiler/preprocessor | |
463 | annotations to print warnings at compile time, pointing to the appropriate | |
464 | update path. A `-Werror` build should fail if compatibility bits are used. | |
a03e3526 QY |
465 | |
466 | ### Miscellaneous | |
467 | ||
468 | When in doubt, follow the guidelines in the Linux kernel style guide, or ask on | |
469 | the development mailing list / public Slack instance. |