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