Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

illumos/dev-guide#40 Update for gerrit, illumos-docs #50

Closed
wants to merge 2 commits into from

Conversation

rmustacc
Copy link
Contributor

@rmustacc rmustacc commented Aug 9, 2023

This overhauls a number of different sections that are out of date, like webrev, etc. It also incorporates parts of #38.

illumos/dev-guid#49 Update glossary page about lint
Portions contributed by: Richard Lowe <[email protected]>
Copy link
Member

@richlowe richlowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just nits

anatomy Outdated
`OBJS` and `SRCS`. We use both throughout the build and to help drive things
like `lint`. In `OBJS` we place a list of our object files. In this case we only
`OBJS`. We use both throughout the build and to help drive
otehr targets. In `OBJS` we place a list of our object files. In this case we only
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, thanks.

anatomy Outdated
before. Finally, we end things with an include of `Makefile.targ` from inside of
From here on out, things stay relatively straightforward. Previously we
didn't worry about a `clean` target, but now that we've generated
intermeidate object files we have to go through and clean them up.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intermediate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, thanks.

anatomy Outdated
variable. The second is to change how we're handling the `lint` target.
Currently the `lint` target is set to `lint_PROG`. We need to change that to
`lint_SRCS`. While the `SRCS` assignment we made in `Makefile.com` earlier may
variable. to While the `SRCS` assignment we made in `Makefile.com` earlier may
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weirdness here, not sure what happened

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftovers from attempting to deal with SRCS which is no longer really required I think.

integrating Outdated
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.
RTI is sent to the the core team. n core team member who is familiar with the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"n core team member"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

integrating Outdated
RTI is sent to the the core team. n 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 and work with to get things integrated. Most of the time, the core
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"work with to"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this is better.

integrating Outdated
Comment on lines 19 to 20
or confirmation about a change. The core team members go through this same
process.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps amend to say "go through this same process when integrating their own changes"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, included.

integrating Outdated

* List of Reviewers

* Explanation of Testing
* Explanation of Testing in the ticket
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is testing capitalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably because when I did this almost a decade ago I was trying to think of it like a section title ala reviewers above. Cleaned up.

integrating Outdated
things between you and your reviewers, that is something that you need to tell
the advocates.
things between you and your reviewers, that is something that you should
mention in the core team. It's okay to still submit the change.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"mention in the core team"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have been "mention to". Fixed.

integrating Outdated
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 tog et back. If a core team member needs time to review the contents of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"tog et"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

Copy link
Contributor Author

@rmustacc rmustacc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pfmooney and @richlowe. I've tried to clean up everything here.

anatomy Outdated
variable. The second is to change how we're handling the `lint` target.
Currently the `lint` target is set to `lint_PROG`. We need to change that to
`lint_SRCS`. While the `SRCS` assignment we made in `Makefile.com` earlier may
variable. to While the `SRCS` assignment we made in `Makefile.com` earlier may
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftovers from attempting to deal with SRCS which is no longer really required I think.

anatomy Outdated
before. Finally, we end things with an include of `Makefile.targ` from inside of
From here on out, things stay relatively straightforward. Previously we
didn't worry about a `clean` target, but now that we've generated
intermeidate object files we have to go through and clean them up.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, thanks.

anatomy Outdated
`OBJS` and `SRCS`. We use both throughout the build and to help drive things
like `lint`. In `OBJS` we place a list of our object files. In this case we only
`OBJS`. We use both throughout the build and to help drive
otehr targets. In `OBJS` we place a list of our object files. In this case we only
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, thanks.

integrating Outdated
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.
RTI is sent to the the core team. n core team member who is familiar with the
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

integrating Outdated
RTI is sent to the the core team. n 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 and work with to get things integrated. Most of the time, the core
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this is better.

integrating Outdated
Comment on lines 19 to 20
or confirmation about a change. The core team members go through this same
process.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, included.

integrating Outdated

* List of Reviewers

* Explanation of Testing
* Explanation of Testing in the ticket
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably because when I did this almost a decade ago I was trying to think of it like a section title ala reviewers above. Cleaned up.

integrating Outdated
things between you and your reviewers, that is something that you need to tell
the advocates.
things between you and your reviewers, that is something that you should
mention in the core team. It's okay to still submit the change.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have been "mention to". Fixed.

integrating Outdated
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 tog et back. If a core team member needs time to review the contents of
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

@rmustacc
Copy link
Contributor Author

Thanks all. I've landed this manually in d8fb997 so I could keep the commit message correct.

@rmustacc rmustacc closed this Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants