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