Folks, Here's a description of the envisioned workflow for using Gitlab as the primary platform for Wine development, should we decide to adopt it. This will probably evolve as we experiment and find out what works or doesn't work for us. All feedback/suggestions are welcome. 1. All patch series are submitted as Merge Requests, so that the proper information can be captured. The usual rules apply (small patches, small number of patches per MR, sign-off by submitter). There are various methods available for creating merge requests: - using the web interface - directly when pushing: git push -o merge_request.create cf. https://docs.gitlab.com/ee/user/project/push_options.html - from the command line, eg. 'glab mr create' cf. https://glab.readthedocs.io/en/latest/mr/create.html - by email cf. https://docs.gitlab.com/ee/user/project/merge_requests/creating_merge_reques... 2. The merge request will run through the CI pipeline, ideally including a testbot run (not implemented yet). 3. The patches of the merge request will be forwarded to wine-devel by the mail gateway, for people who prefer to use their existing email tools to manage patches. 4. Reviewers will be automatically assigned to the merge request based on the MAINTAINERS file (not implemented yet). Reviewers can also be assigned manually. People assigned as reviewers should receive a notification from gitlab. 5. When someone starts reviewing a patch, they can optionally assign the patch to themselves, so that the submitter is aware that someone is working on their patch. 6. Feedback on patches can be sent by replying to the wine-devel email, or by adding a comment on gitlab. The mail gateway will bridge things so that all comments are reflected both on gitlab and on wine-devel. Note: if you subscribe to gitlab notifications (recommended) currently you'll get many duplicate emails. There are mail headers that can be used for filtering, but we may want to handle this differently, suggestions are welcome. 7. If there are comments, the submitter can address them and update the merge request by pushing to the MR branch. The mail gateway will send the new version of the patches to wine-devel (except for trivial changes) for further review. 8. Once a reviewer approves a merge request, they should mark it approved in gitlab. The idea is that this will replace the existing "send a signoff by email" mechanism. Note that an approval applies to the whole merge request, not to individual patches. This should be one more reason to keep patch series small. Approvals can be sent by: - using the approve button on the web interface - replying to a gitlab notification with "/approve" (note: replying to a wine-devel mail won't work, it needs the magic gitlab headers) cf. https://docs.gitlab.com/ee/user/project/quick_actions.html - from the command line, eg. 'glab mr approve' cf. https://glab.readthedocs.io/en/latest/mr/approve.html 9. Once a merge request has been approved by all reviewers, I'll merge it into my tree, do a full testbot run to make sure it doesn't break anything, then push the results and mark the merge request as 'merged'. 10. If a merge request is no longer relevant, the submitter should close it. We may want to add an auto-close process for stale merge requests at some point. -- Alexandre Julliard julliard(a)winehq.org
On Mon, Apr 25, 2022 at 12:04 PM Alexandre Julliard <julliard(a)winehq.org> wrote:
1. All patch series are submitted as Merge Requests, so that the proper information can be captured. The usual rules apply (small patches, small number of patches per MR, sign-off by submitter).
For large patchsets, would it make sense to create a separate draft MR with the full set of changes, so they are available but don't have to be reviewed at once?
"Esme Povirk (she/they)" <esme(a)codeweavers.com> writes:
On Mon, Apr 25, 2022 at 12:04 PM Alexandre Julliard <julliard(a)winehq.org> wrote:
1. All patch series are submitted as Merge Requests, so that the proper information can be captured. The usual rules apply (small patches, small number of patches per MR, sign-off by submitter).
For large patchsets, would it make sense to create a separate draft MR with the full set of changes, so they are available but don't have to be reviewed at once?
I suppose we could do that. We'd probably want the mail gateway to mark the patches are draft/RFC then. -- Alexandre Julliard julliard(a)winehq.org
On 4/26/22 09:15, Alexandre Julliard wrote:
"Esme Povirk (she/they)" <esme(a)codeweavers.com> writes:
On Mon, Apr 25, 2022 at 12:04 PM Alexandre Julliard <julliard(a)winehq.org> wrote:
1. All patch series are submitted as Merge Requests, so that the proper information can be captured. The usual rules apply (small patches, small number of patches per MR, sign-off by submitter).
For large patchsets, would it make sense to create a separate draft MR with the full set of changes, so they are available but don't have to be reviewed at once?
I suppose we could do that. We'd probably want the mail gateway to mark the patches are draft/RFC then.
As far as I can tell, the convention in gitlab is to mark draft MRs with a text prefix of 'Draft:'. That should flow through fine. Also note that any MR with more than 15 patches will not be sent to the mailing list. Instead, a single note with a link to the MR will be sent. Cheers, Jeremy
Hi Alexandre, On 4/25/22 19:04, Alexandre Julliard wrote:
4. Reviewers will be automatically assigned to the merge request based on the MAINTAINERS file (not implemented yet). Reviewers can also be assigned manually. People assigned as reviewers should receive a notification from gitlab.
5. When someone starts reviewing a patch, they can optionally assign the patch to themselves, so that the submitter is aware that someone is working on their patch.
Does this means we're supposed to be able to assign ourselves as reviewers of merge requests or is it still something you only can do? Right now it doesn't seem to be possible (maybe a permission issue, or there's a contribution threshold, or I haven't found the button). I personally think it'd be very nice if it was open to anyone, so that we can indicate we're willing to review some merge request, without having to necessarily be the maintainer of the area first. Of course reviews from non-maintainers won't be the same as reviews from maintainers, but I think it can help with the workload. Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
Rémi Bernon <rbernon(a)codeweavers.com> writes:
Hi Alexandre,
On 4/25/22 19:04, Alexandre Julliard wrote:
4. Reviewers will be automatically assigned to the merge request based on the MAINTAINERS file (not implemented yet). Reviewers can also be assigned manually. People assigned as reviewers should receive a notification from gitlab. 5. When someone starts reviewing a patch, they can optionally assign the patch to themselves, so that the submitter is aware that someone is working on their patch.
Does this means we're supposed to be able to assign ourselves as reviewers of merge requests or is it still something you only can do? Right now it doesn't seem to be possible (maybe a permission issue, or there's a contribution threshold, or I haven't found the button).
Yes that's the intent, it's probably a permission issue. You may need to request access to the project first. -- Alexandre Julliard julliard(a)winehq.org
9. Once a merge request has been approved by all reviewers, I'll merge it into my tree, do a full testbot run to make sure it doesn't break anything, then push the results and mark the merge request as 'merged'.
question: - if mr is sent on day D - approval by maintainer on day D+1 - merge approved in Wine tree day D+2 there can obviously be new commits applies on wine master tree on days D and D+1 (and also D+2 before merge approval) who's in charge of rebasing in gitlab? - author of MR - maintainer (when validating at D+1) - Alexandre when applying... that's not 100% clear to be
Eric Pouech <eric.pouech(a)orange.fr> writes:
9. Once a merge request has been approved by all reviewers, I'll merge it into my tree, do a full testbot run to make sure it doesn't break anything, then push the results and mark the merge request as 'merged'.
question:
- if mr is sent on day D
- approval by maintainer on day D+1
- merge approved in Wine tree day D+2
there can obviously be new commits applies on wine master tree on days D and D+1 (and also D+2 before merge approval)
who's in charge of rebasing in gitlab?
- author of MR
- maintainer (when validating at D+1)
- Alexandre when applying...
that's not 100% clear to be
As long as it rebases cleanly, there's nothing to do, I'll rebase when applying. It's not necessary to rebase the MR in gitlab after every commit round. If there are conflicts, then it would be up to the author to rebase and push an updated MR. In that case the MR should be reassigned to the author. -- Alexandre Julliard julliard(a)winehq.org
On Mon, Apr 25, 2022 at 12:04 PM Alexandre Julliard <julliard(a)winehq.org> wrote:
1. All patch series are submitted as Merge Requests, so that the proper information can be captured. The usual rules apply (small patches, small number of patches per MR, sign-off by submitter).
There are various methods available for creating merge requests: - using the web interface - directly when pushing: git push -o merge_request.create cf. https://docs.gitlab.com/ee/user/project/push_options.html - from the command line, eg. 'glab mr create' cf. https://glab.readthedocs.io/en/latest/mr/create.html - by email cf. https://docs.gitlab.com/ee/user/project/merge_requests/creating_merge_reques...
I take it, then, that MRs should have changes to generated files (e.g. autoconf scripts) in them? I know the long-standing policy of this project was that patches *shouldn't* include generated files in them, because you would regenerate the files when you committed the patches. -- Chip
Charles Davis <cdavis5x(a)gmail.com> writes:
On Mon, Apr 25, 2022 at 12:04 PM Alexandre Julliard <julliard(a)winehq.org> wrote:
1. All patch series are submitted as Merge Requests, so that the proper information can be captured. The usual rules apply (small patches, small number of patches per MR, sign-off by submitter).
There are various methods available for creating merge requests: - using the web interface - directly when pushing: git push -o merge_request.create cf. https://docs.gitlab.com/ee/user/project/push_options.html - from the command line, eg. 'glab mr create' cf. https://glab.readthedocs.io/en/latest/mr/create.html - by email cf. https://docs.gitlab.com/ee/user/project/merge_requests/creating_merge_reques...
I take it, then, that MRs should have changes to generated files (e.g. autoconf scripts) in them? I know the long-standing policy of this project was that patches *shouldn't* include generated files in them, because you would regenerate the files when you committed the patches.
I'm doing it the same way for merge requests, so generated files don't need to be included either. -- Alexandre Julliard julliard(a)winehq.org
participants (6)
-
Alexandre Julliard -
Charles Davis -
Eric Pouech -
Esme Povirk (she/they) -
Jeremy White -
Rémi Bernon