forked from illumos/dev-guide
-
Notifications
You must be signed in to change notification settings - Fork 0
/
Copy pathintegrating
195 lines (147 loc) · 9.05 KB
/
integrating
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
# Integrating Changes
## Introduction
This section of the developer's guide covers the integration process, what you
need to do before you integrate, and what you need to provide for integration.
This will also cover best practices and suggestions around testing as well as
tips and advice to prove a smooth integration process.
## RTI process
When you want to submit a change into illumos, we call that a request to
integrate, or RTI. illumos has no single person that serves as a gatekeeper. An
RTI is sent to the advocates. An advocate who is familiar with the subject
matter will review the materials and either accept or reject the RTI. If it is
accepted, the advocate will push those change into illumos. If it is rejected,
the advocate will explain why, for example inadequate testing, and you can
return when you have corrected those issues. The advocates is made up of members
of the community and while they have commit access to the gate, they are also
required to go through the same process.
### The Requirements
When you come to the advocates, you are required to have a few different pieces
of information. We'll start off by listing them now, and go into more detail a
bit later about each of them.
* List of Reviewers
* Explanation of Testing
* The output of `git pbchk`
* Nightly build `mail_msg`
* The change itself (output of `git format-patch`)
### Shrink to Fit
There is a fair bit there, but this whole process is designed to be shrink to
fit. If because of what you're working on there are parts of the above list
which do not apply, then you should not include them. For example, if you are
fixing a typo in a manual page, then you do not need to do a full nightly build
and the only testing you need to do is to make sure that you didn't add any
rendering pages to the new manual page.
At the end of the day, it is the advocates who are responsible for determining
whether or not something is sufficient. If you have opted to pass on one of the
requirements believing that it is not necessary, but the advocate feels that it
does, then you must address that.
### Review
Having reviewers for your change is one of the more important aspects of the RTI
process. Review is used to not only be a sanity check, but a defense against
broken code. Changes must have at least one reviewer, but the number is less
important than the quality of said reviewers.
Many of the original authors of various kernel subsystems and user land
components are involved in the illumos community. Having a single reviewer who
intimately knows the workings of a given subsystem is invaluable. For example,
if you're making a change to ZFS, but none of your reviewers has ever done any
work in ZFS before, then that may be a warning sign to your advocate.
If a reviewer is not satisfied, you may not list them as a reviewer when
submitting your RTI. Furthermore, if there are issues or disagreements about
things between you and your reviewers, that is something that you need to tell
the advocates.
When providing materials for review, the best form of this is a `webrev`. For
information on generating it, please read [the workflow section on
webrev](./workflow.html#webrev). If you have written custom testing software,
you should make that available. This will not only help reviewers better
understand how your changes work, but they will also be able to provide feedback
on the testing and may be able to find cracks in the test plan.
#### Finding Reviewers
Sometimes it can be challenging to find reviewers. Often times there may be no
subject matter expert in the community. If you're uncertain of who should review
your work, you can send mail to the illumos developer list
`[email protected]`. There exist a large number of people on the list
who are willing to help review work. For complicated or large changes, you'll
want to give your reviewers plenty of time to do their work. Thorough reviews
are complicated.
illumos also has sublists for different aspects of the system. If your change
deals with that area, you should consider sending it to that list. While there
are less people on these lists, they generally have more familiarity and
expertise with that specific area of code. These lists include:
* `[email protected]` for ZFS related changes
* `[email protected]` for DTrace related changes
* `[email protected]` for networking related changes.
There is no requirement for sending mail to a public list for review if you
already feel that you have adequate reviewers. Keep in mind that in addition to
ensuring your changes are correct, part of the point of soliciting review and
having reviewers who are familiar with the area is to aid in showing your
advocate that your changes are correct.
### Testing
You are required to do enough testing such that not only you, but others can be
confident in your changes. Testing is one place where it's really shrink to fit.
For example, the amount of testing involved for adding a new argument to the
command `head` is much less than making a change to ZFS. As part of the RTI
process, you will need to have a description of *how* you tested it. The how
aspect is what your advocate cares about, it's assumed that your tests pass.
#### Test Suites
Several components already have a full and involved test suite such as the ZFS
and DTrace test suites. If you're working in an area where that already exists,
you should run those test suites to help you gain confidence that not only does
your code work, but that you have not introduced any regressions.
If you write a test suite, you should consider including it into the gate. This
will allow others to benefit from your work and further allow us to help ensure
that no regressions have been introduced as a side effect of another change.
#### Other Testing Areas
There are several areas that often get overlooked for testing. If you're working
in a daemon or a kernel subsystem, you'll want to run a debug build and check
for memory leaks in the kernel via a crash dump, `mdb(1)`, and `::findleaks`, or
via `libumem` when working with user land programs and daemons.
If you are working in an area that is performance sensitive you should gather
performance data on a non-debug build. You should be able to convince yourself
that there are no regressions as a side effect of your change.
### git pbchk
Part of the material that you need to submit includes the output of `git pbchk`.
As discussed in the [workflow section](./workflow.html#pbchk), you should be
`git pbchk` clean.
### nightly `mail_msg`
If you are making changes to code, you should provide the output of a full
nightly. You should not run an incremental nightly. It is important that you
have not introduced any build noise that is caught by lint and the make check
targets. Similarly, if you have touched anything to do with pacakges, you must
ensure that your nightly flags build the packages and that you run a `protocmp`
which checks everything that is in the proto area.
### Delta
The advocates are going to apply your changes to the gate. As we are using
`git`, the best way to do this is by attaching the output of `git format-patch`.
While `webrev` generates a patch, it isn't always the most useful, as it leads
to people trying to manually add missing files which can be an error prone
process.
If isn't required for you to have merged your changes into the head of the tree,
but if your advocate is doing so and runs into substantial trouble with that,
they may come back to you and ask you to sync up your changes and provide an
updated patch.
## Submitting
Once you have prepared all of the different components, it's time to submit your
change to the advocates list. You should prepare an e-mail and address it to
`[email protected]` with a subject line that describes the changes
that you wish to integrate, for example, use a list of the bug ids or a subject
which describes the work you're doing such as 'libproc enhancements'. You should
include the following in the e-mail:
* The list of reviewers (commonly folks use the commit message for this)
* A description of the testing and links to the tests if applicable
* The output of `git pbchk`
You should attach the following in your e-mail:
* The `git format-patch` for your change
* The nightly `mail_msg`
You should expect the advocates to get back to you promptly, in general give
them a day before you nag them.
### When Advocates Drop the Ball
Like the rest of us, the advocates are human, and occasionally a given change
might fall through the cracks. It could be that the advocate most familiar with
your area. If an advocate needs time to review the contents of the change or has
questions regarding the nature of it and requests more time, that advocate
should tell you that. The advocates do not want to have changes feel like they
are sitting in limbo.
If you don't hear from them within a day or two, send a friendly reminder as a
reply to that e-mail. If an advocate is free in `#illumos` on IRC, politely ask
them. Please remember to send a given change to all of the advocates. If you
send a given change to just one of them, that is a recipe for having your change
get dropped.