]>
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 | |
53c2f1b7 QY |
225 | ### Commit messages |
226 | ||
227 | Commit messages should be formatted in the same way as Linux kernel commit | |
228 | messages. The format is roughly | |
229 | ||
230 | ``` dir: short summary | |
231 | ||
232 | extended summary ``` | |
233 | ||
234 | `dir` should be the top level source directory under which the change was made. | |
235 | For example, a change in bgpd/rfapi would be formatted as: | |
236 | ||
237 | `bgpd: short summary` | |
238 | ||
239 | The first line should be no longer than 50 characters. Subsequent lines should | |
240 | be wrapped to 72 characters. | |
241 | ||
c545559d | 242 | ### Source file header |
f6ee5b52 | 243 | |
4765f35e DL |
244 | New files need to have a Copyright header (see [License for |
245 | contributions](#license-for-contributions) above) added to the file. Preferred | |
246 | form of the header is as follows: | |
f6ee5b52 DL |
247 | |
248 | ``` | |
249 | /* | |
896014f4 DL |
250 | * Title/Function of file |
251 | * Copyright (C) YEAR Author’s Name | |
252 | * | |
253 | * This program is free software; you can redistribute it and/or modify it | |
254 | * under the terms of the GNU General Public License as published by the Free | |
255 | * Software Foundation; either version 2 of the License, or (at your option) | |
256 | * any later version. | |
257 | * | |
258 | * This program is distributed in the hope that it will be useful, but WITHOUT | |
259 | * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | |
260 | * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for | |
261 | * more details. | |
262 | * | |
263 | * You should have received a copy of the GNU General Public License along | |
264 | * with this program; see the file COPYING; if not, write to the Free Software | |
265 | * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA | |
f6ee5b52 | 266 | */ |
4765f35e DL |
267 | |
268 | #include <zebra.h> | |
f6ee5b52 DL |
269 | ``` |
270 | ||
c545559d | 271 | ### Adding copyright claims to existing files |
f6ee5b52 DL |
272 | |
273 | When adding copyright claims for modifications to an existing file, please | |
274 | preface the claim with "Portions: " on a line before it and indent the | |
275 | "Copyright ..." string. If such a case already exists, add your indented claim | |
276 | immediately after. E.g.: | |
277 | ||
278 | ``` | |
279 | Portions: | |
280 | Copyright (C) 2010 Entity A .... | |
281 | Copyright (C) 2016 Your name [optional brief change description] | |
282 | ``` | |
283 | ||
c545559d | 284 | ### Code formatting |
f6ee5b52 | 285 | |
b0ff7312 QY |
286 | FRR uses Linux kernel style except where noted below. Code which does not |
287 | comply with these style guidelines will not be accepted. | |
02fe6f86 | 288 | |
b0ff7312 | 289 | To assist with compliance, in the project root there is a .clang-format |
6058ea8c QY |
290 | configuration file which can be used with the `clang-format` tool from the LLVM |
291 | project. In the `tools/` directory there is a Python script named `indent.py` | |
292 | that wraps clang-format and handles some edge cases specific to FRR. If you are | |
293 | submitting a new file, it is recommended to run that script over the new file | |
294 | after ensuring that the latest stable release of `clang-format` is in your | |
295 | PATH. | |
02fe6f86 DL |
296 | |
297 | **Whitespace changes in untouched parts of the code are not acceptable in | |
298 | patches that change actual code.** To change/fix formatting issues, please | |
299 | create a separate patch that only does formatting changes and nothing else. | |
300 | ||
b0ff7312 | 301 | #### Style documentation |
6058ea8c | 302 | Kernel and BSD styles are documented externally: |
02fe6f86 | 303 | |
6058ea8c QY |
304 | * [https://www.kernel.org/doc/html/latest/process/coding-style.html](https://www.kernel.org/doc/html/latest/process/coding-style.html) |
305 | * [http://man.openbsd.org/style](http://man.openbsd.org/style) | |
02fe6f86 | 306 | |
6058ea8c | 307 | For GNU coding style, use `indent` with the following invocation: |
f6ee5b52 DL |
308 | |
309 | ``` | |
310 | indent -nut -nfc1 file_for_submission.c | |
311 | ``` | |
312 | ||
6058ea8c QY |
313 | #### Exceptions |
314 | ||
315 | FRR project code comes from a variety of sources, so there are some stylistic | |
c545559d | 316 | exceptions in place. They are organized here by branch. |
02fe6f86 | 317 | |
6058ea8c | 318 | **For `master`:** |
02fe6f86 | 319 | |
6058ea8c | 320 | BSD coding style applies to: |
02fe6f86 | 321 | |
c545559d | 322 | * `ldpd/` |
02fe6f86 | 323 | |
6058ea8c QY |
324 | `babeld` uses, approximately, the following style: |
325 | ||
326 | * K&R style braces | |
327 | * Indents are 4 spaces | |
328 | * Function return types are on their own line | |
329 | ||
330 | ||
331 | **For `stable/3.0` and `stable/2.0`:** | |
332 | ||
333 | GNU coding style apply to the following parts: | |
334 | ||
c545559d QY |
335 | * `lib/` |
336 | * `zebra/` | |
337 | * `bgpd/` | |
338 | * `ospfd/` | |
339 | * `ospf6d/` | |
340 | * `isisd/` | |
341 | * `ripd/` | |
342 | * `ripngd/` | |
343 | * `vtysh/` | |
02fe6f86 | 344 | |
6058ea8c | 345 | BSD coding style applies to: |
02fe6f86 | 346 | |
c545559d | 347 | * `ldpd/` |
02fe6f86 | 348 | |
02fe6f86 | 349 | |
a03e3526 QY |
350 | ### Documentation |
351 | ||
352 | FRRouting is a large and complex software project developed by many different | |
353 | people over a long period of time. Without adequate documentation, it can be | |
354 | exceedingly difficult to understand code segments, APIs and other interfaces. | |
355 | In the interest of keeping the project healthy and maintainable, you should | |
356 | make every effort to document your code so that other people can understand | |
357 | what it does without needing to closely read the code itself. | |
358 | ||
359 | Some specific guidelines that contributors should follow are: | |
360 | ||
361 | * Functions exposed in header files should have descriptive comments above | |
362 | their signatures in the header file. At a minimum, a function comment should | |
363 | contain information about the return value, parameters, and a general summary | |
364 | of the function's purpose. Documentation on parameter values can be omitted | |
365 | if it is (very) obvious what they are used for. | |
366 | ||
367 | Function comments must follow the style for multiline comments laid out in | |
368 | the kernel style guide. | |
369 | ||
370 | Example: | |
371 | ||
372 | ``` | |
373 | /* | |
374 | * Determines whether or not a string is cool. | |
375 | * | |
376 | * @param text - the string to check for coolness | |
377 | * @param is_clccfc - whether capslock is cruise control for cool | |
378 | * @return 7 if the text is cool, 0 otherwise | |
379 | */ | |
380 | int check_coolness(const char *text, bool is_clccfc); | |
381 | ``` | |
382 | ||
383 | The Javadoc-style annotations are not required, but you should still strive to | |
384 | make it equally clear what parameters and return values are used for. | |
385 | ||
386 | * Static functions should have descriptive comments in the same form as above | |
387 | if what they do is not immediately obvious. Use good engineering judgement | |
388 | when deciding whether a comment is necessary. If you are unsure, document | |
389 | your code. | |
390 | ||
391 | * Global variables, static or not, should have a comment describing their use. | |
392 | ||
393 | * **For new code in `lib/`, these guidelines are hard requirements.** | |
394 | ||
395 | ||
396 | If you are contributing code that adds significant user-visible functionality | |
397 | or introduces a new API, please document it in `doc/`. Markdown and LaTeX are | |
398 | acceptable formats, although Markdown is currently preferred for new | |
399 | documentation. This may change in the near future. | |
400 | ||
401 | Finally, if you come across some code that is undocumented and feel like going | |
402 | above and beyond, document it! We absolutely appreciate and accept patches that | |
403 | document previously undocumented code. | |
f6ee5b52 | 404 | |
c545559d | 405 | ### Compile-time conditional code |
f6ee5b52 | 406 | |
f1423462 QY |
407 | Many users access FRR via binary packages from 3rd party sources; compile-time |
408 | code puts inclusion/exclusion in the hands of the package maintainer. Please | |
409 | think very carefully before making code conditional at compile time, as it | |
410 | increases regression testing, maintenance burdens, and user confusion. In | |
411 | particular, please avoid gratuitous `--enable-…` switches to the configure | |
412 | script - in general, code should be of high quality and in working condition, | |
413 | or it shouldn’t be in FRR at all. | |
f6ee5b52 DL |
414 | |
415 | When code must be compile-time conditional, try have the compiler make it | |
c545559d QY |
416 | conditional rather than the C pre-processor so that it will still be checked by |
417 | the compiler, even if disabled. For example, | |
f6ee5b52 DL |
418 | |
419 | ``` | |
420 | if (SOME_SYMBOL) | |
421 | frobnicate(); | |
422 | ``` | |
423 | ||
c545559d | 424 | is preferred to |
f6ee5b52 DL |
425 | |
426 | ``` | |
427 | #ifdef SOME_SYMBOL | |
428 | frobnicate (); | |
429 | #endif /* SOME_SYMBOL */ | |
430 | ``` | |
431 | ||
432 | Note that the former approach requires ensuring that `SOME_SYMBOL` will be | |
433 | defined (watch your `AC_DEFINE`s). | |
7ce0cb3c | 434 | |
c545559d | 435 | ### Debug-guards in code |
7ce0cb3c | 436 | |
f1423462 QY |
437 | Debugging statements are an important methodology to allow developers to fix |
438 | issues found in the code after it has been released. The caveat here is that | |
439 | the developer must remember that people will be using the code at scale and in | |
440 | ways that can be unexpected for the original implementor. As such debugs | |
441 | **MUST** be guarded in such a way that they can be turned off. FRR has the | |
442 | ability to turn on/off debugs from the CLI and it is expected that the | |
443 | developer will use this convention to allow control of their debugs. | |
19c7f43f | 444 | |
c545559d | 445 | ### CLI changes |
19c7f43f | 446 | |
f1423462 QY |
447 | CLI's are a complicated ugly beast. Additions or changes to the CLI should use |
448 | a DEFUN to encapsulate one setting as much as is possible. Additionally as new | |
449 | DEFUN's are added to the system, documentation should be provided for the new | |
450 | commands. | |
e887b2b8 LB |
451 | |
452 | ### Backwards Compatibility | |
453 | ||
f1423462 QY |
454 | As a general principle, changes to CLI and code in the lib/ directory should be |
455 | made in a backwards compatible fashion. This means that changes that are purely | |
456 | stylistic in nature should be avoided, e.g., renaming an existing macro or | |
457 | library function name without any functional change. When adding new parameters | |
458 | to common functions, it is also good to consider if this too should be done in | |
459 | a backward compatible fashion, e.g., by preserving the old form in addition to | |
e887b2b8 LB |
460 | adding the new form. |
461 | ||
f1423462 QY |
462 | This is not to say that minor or even major functional changes to CLI and |
463 | common code should be avoided, but rather that the benefit gained from a change | |
464 | should be weighed against the added cost/complexity to existing code. Also, | |
465 | that when making such changes, it is good to preserve compatibility when | |
466 | possible to do so without introducing maintenance overhead/cost. It is also | |
467 | important to keep in mind, existing code includes code that may reside in | |
468 | private repositories (and is yet to be submitted) or code that has yet to be | |
469 | migrated from Quagga to FRR. | |
817302b8 DL |
470 | |
471 | That said, compatibility measures can (and should) be removed when either: | |
472 | ||
f1423462 QY |
473 | * they become a significant burden, e.g. when data structures change and the |
474 | compatibility measure would need a complex adaptation layer or becomes | |
817302b8 | 475 | flat-out impossible |
f1423462 QY |
476 | * some measure of time (dependent on the specific case) has passed, so that the |
477 | compatibility grace period is considered expired. | |
817302b8 DL |
478 | |
479 | In all cases, compatibility pieces should be marked with compiler/preprocessor | |
480 | annotations to print warnings at compile time, pointing to the appropriate | |
481 | update path. A `-Werror` build should fail if compatibility bits are used. | |
a03e3526 QY |
482 | |
483 | ### Miscellaneous | |
484 | ||
485 | When in doubt, follow the guidelines in the Linux kernel style guide, or ask on | |
486 | the development mailing list / public Slack instance. |