-
Notifications
You must be signed in to change notification settings - Fork 16
/
integrating
185 lines (139 loc) · 8.84 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
# 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 core team. A core team member who is familiar with the
subject matter will review the materials and either accept it or not. If the
change isn't accepted for whatever reason, then the core team member will
explain why. That team member will work with you to get things fixed up. Most of
the time, the core team members are just looking for some small amount of
additional explanation or confirmation about a change. The core team members go
through this same process when integrating their own changes.
### The Requirements
When you want to integrate a change, it should include the following information:
* A list of reviewers
* An explanation of testing in the corresponding tickets
* The output of `git pbchk`
* Nightly build `mail_msg`
* The change itself (usually a link to a review on [gerrit](https://illumos.org/docs/contributing/gerrit/)
### 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 core team 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 core team member 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 should
mention to the core team. It's okay to still submit the change.
When providing materials for review, the best thing to use here is
[gerrit](https://illumos.org/docs/contributing/gerrit/). 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
`developer@lists.illumos.org`. 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. If you're having trouble finding reviewers, you can always
reach out to the core team at `advocates@lists.illumos.org` or just reach out
in IRC.
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 take a look.
### 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 the core team members cares about, it's assumed that your tests
pass.
This information should be recorded in the tickets. This is a useful record
when someone next is working in this area and wants to consider what to test.
It also helps in case we discover issues in the future and want to understand
what was and wasn't working in the past.
#### 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; however, copyright comments are always optional..
### 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.
### The Change Itself
To integrate a change, we're going to need the source code change itself. The
simplest way to do this is to link directly to a gerrit review. Otherwise,
attach the output of `git format-patch`.
## Submitting
Once you have prepared all of the different components, it's time to submit your
change to the core team list. You should prepare an e-mail and address it to
`advocates@lists.illumos.org` 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 or link to the review on gerrit)
* 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, if not linking directly to gerrit
* The nightly `mail_msg`
You should expect the core team to get back to you promptly, in general give
them a day before you nag them.
### When the Core Team Drop the Ball
Like the rest of us, members of the core team are human, and occasionally a
given change might fall through the cracks. It could be that the core team
member most familiar with your area is not available and someone is waiting for
them to get back. If a core team member needs time to review the contents of
the change or has questions regarding the nature of it and requests more time,
that core team member should tell you that. The core team does 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 a core team member is free in `#illumos` on IRC,
politely ask them. Please remember to send a given change to the core team
alias (`advocates@lists.illumos.org`). If you send a given change to just one
of them, that is a recipe for having your change get dropped.