Questions about Workflow & Submitting Patches

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Questions about Workflow & Submitting Patches

via cfe-dev
Hello,

I'm working with a team of students that's interested in contributing to the clang front end (with the initial step of writing new checks for clang-tidy). 

We'd like to setup a workflow that makes it easy for us to review each other's work before officially submitting changes upstream, and also will be amenable to officially integrating our changes to clang-tidy.

Our current plan is to fork the git mirror of LLVM, and make commits to this copy of the repo on GitHub. After we finish writing and reviewing a check, we'd get the diff and submit a patch through Phabricator. 

This seems inline with the process suggested in the LLVM docs (from https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-git-svn and https://llvm.org/docs/Phabricator.html). 

Questions
  • Does this workflow make sense, and is it in accordance with LLVM developer policy?
  • Is there an alternate (or more standard) workflow that anyone thinks would work better?
  • If we work through a git repo, is it necessary to use git-svn to make sure svn (and not just git) has changes saved? 
  • Are there any specific practices we should adopt to make sure our additions can be successfully integrated into the current version of LLVM?
Best,
Shyan

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about Workflow & Submitting Patches

via cfe-dev
Dear Shyan,

I am in the same situation, although with a bigger history on this. At
our university, we also have a lab course where students can choose
large-scale projects involving research and industrial work for part
of their studies, one such is heavily about C++ and a big cornerstone
of that is LLVM. I am an instructor to this project here.

Basically, our workflow is the same and has been for a couple of years now.

Our current workflow is as follows: we have a locally hosted GitLab
system where students in this project are given access. We have a
mirror that mirrors the latest *release* of LLVM so that a stable
branching point is given. Brainstorming of checker ideas and initial
implementation for students is done here. We >purely< use Git (this is
also somewhat pedagogical because most freshly graduated students have
zero knowledge of teamwork and VCSs) on this local thing. Each student
has a fork of this repository, and they work there.
(Disclaimer: I'm not affiliated with GitLab in any way, just from a
user and "system administrator" perspective: their system is pretty
neat. It's easy to install, runs without any hassle with virtual
images and such, sleek to configure, doesn't use many resources. The
bare minimum version (which already supports a lot of basic Git
flows!) is free to install on your own computers, but I'm sure if you
want to also integrate other more fancy CI/CD kinds of stuff -- not
necessary for students -- you could get a nice discount as an academic
institution.)

First thing first is when an idea is fleshed out (and isn't too
extreme, mathematically infeasible, or outright bull) is that we
encourage the student to investigate a bit: write a few tests of the
good and bad behaviour, check for warnings, already existing checkers
in SA and Tidy, in other tools such as CppCheck, do a quick look if a
similar implementation effort is ongoing, etc. (Of course we also
monitor the available channels to prevent useless work, but you need
to keep in mind that pedagogical effort and industrial effort must be
in balance here.)

This is purely just a suggestion to them. More knowledgeable or
adventurous students are encouraged to work off of the "true" master
mirror available on GitHub: https://github.com/llvm-mirror/clang, and
maybe even have their fork on GitHub, if they want. The GitLab is just
a closer, more neighbour-y environment so that students whose first
"real deal" is writing a checker isn't thrown into the Pacific Ocean.
I think you can pretty much survive without ever having to touch SVN
or Git-SVN, because this mirror on GitHub is nicely up-to-date, and
works just like any other Git repository.

Students are encouraged to work and consult with us regularly and to
use the usual "forking" and "topic branch" workflows commonly
available in Git.

Once their implementation is in a presentable state, we do something
sort of an inner review. This is basically a GitLab merge request in
our local system, where we look at their code. This is done to ensure
the feasibility of things, that the most common style and code design
mistakes are handled, that tests are ample, etc. At this point, we
discuss things in English (although personal conversations are usually
kept to our native language) in writing, but of course, there is this
"ease" that on the lab consultation lesson you can walk up to one of
us instructors and ask about the code, etc.

Because at this point they have seemingly used Git extensively, we
have also introduced them to the review process. My personal opinion
is that GitLab/GitHub offers a much more friendly (not necessarily
user-friendly in the technical sense, though!) discussion experience
than Phabricator or Gerrit. (The whole thing of Phab and Gerrit not
being able to keep track of line changes and using patch files as
opposed to the Git workflow just makes it wonky from one end, but more
general from the other.)

After this "inner review" is done, we usually introduce them to the
biggest bane of all Git works: rebasing. If their checker works on the
local fork (whichever latest release of master they did), we, of
course, require them to do a rebase, check up and maybe extend the
tests a bit, and then create the patch on Phabricator.
Then their fate is in the world's hands. ;)

Note that this workflow only applies in a pedagogical way, for a
student's first one or two implementations. Once we see and they feel
that they are capable of tackling the work with a bit less
hand-holding, we don't punish them (actually we don't punish them for
this before they feel capable either!) for going out "into the wild".
So now they are taught -- and they proved -- they know how to wrangle
Git, how to handle patches, maybe even help them in writing English a
bit, and then they are free to continue their work on the latest and
greatest versions.

Regards,
Whisperity.
Shyan Akmal via cfe-dev <[hidden email]> ezt írta (időpont:
2018. okt. 16., K, 4:14):

>
> Hello,
>
> I'm working with a team of students that's interested in contributing to the clang front end (with the initial step of writing new checks for clang-tidy).
>
> We'd like to setup a workflow that makes it easy for us to review each other's work before officially submitting changes upstream, and also will be amenable to officially integrating our changes to clang-tidy.
>
> Our current plan is to fork the git mirror of LLVM, and make commits to this copy of the repo on GitHub. After we finish writing and reviewing a check, we'd get the diff and submit a patch through Phabricator.
>
> This seems inline with the process suggested in the LLVM docs (from https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-git-svn and https://llvm.org/docs/Phabricator.html).
>
> Questions:
>
> Does this workflow make sense, and is it in accordance with LLVM developer policy?
> Is there an alternate (or more standard) workflow that anyone thinks would work better?
> If we work through a git repo, is it necessary to use git-svn to make sure svn (and not just git) has changes saved?
> Are there any specific practices we should adopt to make sure our additions can be successfully integrated into the current version of LLVM?
>
> Best,
> Shyan
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about Workflow & Submitting Patches

via cfe-dev
On Tue, Oct 16, 2018 at 1:55 AM, Whisperity via cfe-dev <[hidden email]> wrote:
After this "inner review" is done, we usually introduce them to the
biggest bane of all Git works: rebasing. If their checker works on the
local fork (whichever latest release of master they did), we, of
course, require them to do a rebase

It's not like this is any easier with a different CVS. Certainly not with svn. Git just lets you find the problems faster :-)

I *highly* recommend git-imerge https://github.com/mhagger/git-imerge

With simple "git rebase" you learn that your commit conflicts with *something* in the firehose of maybe thousands of commits on master. But which one? No idea.

With a git-imerge rebase you learn precisely *which* master commit conflicts with precisely which of your local commits. You see the commit messages of both commits, so you know what was trying to be achieved. And there are no other diffs unrelated to the two conflicting commits.

git-imerge takes a bit longer than a standard git rebase, but it's mostly your computer doing the work, not you. You can go to lunch (or go to sleep) and then fix the problems it found.

 # unrelated to git-imerge, recommended with normal merge/rebase too
git config merge.conflictstyle diff3

git checkout master
git imerge start --name=my-new-rebased-branch --goal=rebase my-branch

# if merge error
# fix the problems. Compile, test, etc

git add ...
git imerge continue

git imerge finish
# test your new branch more extensively then rename branches as desired


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about Workflow & Submitting Patches

via cfe-dev
In reply to this post by via cfe-dev
On Tue, Oct 16, 2018 at 4:14 AM Shyan Akmal <[hidden email]> wrote:
Hello,

I'm working with a team of students that's interested in contributing to the clang front end (with the initial step of writing new checks for clang-tidy). 

We'd like to setup a workflow that makes it easy for us to review each other's work before officially submitting changes upstream, and also will be amenable to officially integrating our changes to clang-tidy.

My advice would be to not do that, but to do all reviews publicly in phabricator. Otherwise the reviewer that accepts the patch will potentially go into all the same questions again that were discussed earlier, and the later you get architectural feedback, the more expensive to change it, so the chance to get insight from folks as early as possible is a huge benefit.

Generally, I'd want to teach students that one of the most important rules of software development is that change is cheaper the earlier it is made, so discovering what needs to change as early as possible is probably the single most important skill one can develop early in one's career :)
 
Our current plan is to fork the git mirror of LLVM, and make commits to this copy of the repo on GitHub. After we finish writing and reviewing a check, we'd get the diff and submit a patch through Phabricator. 

This seems inline with the process suggested in the LLVM docs (from https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-git-svn and https://llvm.org/docs/Phabricator.html). 

Questions
  • Does this workflow make sense, and is it in accordance with LLVM developer policy?
  • Is there an alternate (or more standard) workflow that anyone thinks would work better?
  • If we work through a git repo, is it necessary to use git-svn to make sure svn (and not just git) has changes saved? 
  • Are there any specific practices we should adopt to make sure our additions can be successfully integrated into the current version of LLVM?
Best,
Shyan

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about Workflow & Submitting Patches

via cfe-dev
Thank you everyone for the suggestions! The details in the workflow suggested by Whisperity seem like they'll be very helpful to us moving forward. 

Manuel's point of keeping all reviews public makes a lot of sense. Are there any general guidelines on how large we can make a single patch? In general I know that keeping changes smaller is better, but is it fine to write an entirely new clang-tidy check, and then submit that as a single patch? 


On Tue, Oct 16, 2018 at 2:29 AM Manuel Klimek <[hidden email]> wrote:
On Tue, Oct 16, 2018 at 4:14 AM Shyan Akmal <[hidden email]> wrote:
Hello,

I'm working with a team of students that's interested in contributing to the clang front end (with the initial step of writing new checks for clang-tidy). 

We'd like to setup a workflow that makes it easy for us to review each other's work before officially submitting changes upstream, and also will be amenable to officially integrating our changes to clang-tidy.

My advice would be to not do that, but to do all reviews publicly in phabricator. Otherwise the reviewer that accepts the patch will potentially go into all the same questions again that were discussed earlier, and the later you get architectural feedback, the more expensive to change it, so the chance to get insight from folks as early as possible is a huge benefit.

Generally, I'd want to teach students that one of the most important rules of software development is that change is cheaper the earlier it is made, so discovering what needs to change as early as possible is probably the single most important skill one can develop early in one's career :)
 
Our current plan is to fork the git mirror of LLVM, and make commits to this copy of the repo on GitHub. After we finish writing and reviewing a check, we'd get the diff and submit a patch through Phabricator. 

This seems inline with the process suggested in the LLVM docs (from https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-git-svn and https://llvm.org/docs/Phabricator.html). 

Questions
  • Does this workflow make sense, and is it in accordance with LLVM developer policy?
  • Is there an alternate (or more standard) workflow that anyone thinks would work better?
  • If we work through a git repo, is it necessary to use git-svn to make sure svn (and not just git) has changes saved? 
  • Are there any specific practices we should adopt to make sure our additions can be successfully integrated into the current version of LLVM?
Best,
Shyan

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about Workflow & Submitting Patches

via cfe-dev
On Wed, Oct 17, 2018 at 1:41 AM Shyan Akmal <[hidden email]> wrote:
Thank you everyone for the suggestions! The details in the workflow suggested by Whisperity seem like they'll be very helpful to us moving forward. 

Manuel's point of keeping all reviews public makes a lot of sense. Are there any general guidelines on how large we can make a single patch? In general I know that keeping changes smaller is better, but is it fine to write an entirely new clang-tidy check, and then submit that as a single patch? 

I think most clang-tidy patches come as a single patch. When the patch is too large people will usually tell you to split it up :)
 


On Tue, Oct 16, 2018 at 2:29 AM Manuel Klimek <[hidden email]> wrote:
On Tue, Oct 16, 2018 at 4:14 AM Shyan Akmal <[hidden email]> wrote:
Hello,

I'm working with a team of students that's interested in contributing to the clang front end (with the initial step of writing new checks for clang-tidy). 

We'd like to setup a workflow that makes it easy for us to review each other's work before officially submitting changes upstream, and also will be amenable to officially integrating our changes to clang-tidy.

My advice would be to not do that, but to do all reviews publicly in phabricator. Otherwise the reviewer that accepts the patch will potentially go into all the same questions again that were discussed earlier, and the later you get architectural feedback, the more expensive to change it, so the chance to get insight from folks as early as possible is a huge benefit.

Generally, I'd want to teach students that one of the most important rules of software development is that change is cheaper the earlier it is made, so discovering what needs to change as early as possible is probably the single most important skill one can develop early in one's career :)
 
Our current plan is to fork the git mirror of LLVM, and make commits to this copy of the repo on GitHub. After we finish writing and reviewing a check, we'd get the diff and submit a patch through Phabricator. 

This seems inline with the process suggested in the LLVM docs (from https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-git-svn and https://llvm.org/docs/Phabricator.html). 

Questions
  • Does this workflow make sense, and is it in accordance with LLVM developer policy?
  • Is there an alternate (or more standard) workflow that anyone thinks would work better?
  • If we work through a git repo, is it necessary to use git-svn to make sure svn (and not just git) has changes saved? 
  • Are there any specific practices we should adopt to make sure our additions can be successfully integrated into the current version of LLVM?
Best,
Shyan

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about Workflow & Submitting Patches

via cfe-dev


On 17 Oct 2018 10:09, "Manuel Klimek via cfe-dev" <[hidden email]> wrote:
On Wed, Oct 17, 2018 at 1:41 AM Shyan Akmal <[hidden email]> wrote:
Thank you everyone for the suggestions! The details in the workflow suggested by Whisperity seem like they'll be very helpful to us moving forward. 

Manuel's point of keeping all reviews public makes a lot of sense. Are there any general guidelines on how large we can make a single patch? In general I know that keeping changes smaller is better, but is it fine to write an entirely new clang-tidy check, and then submit that as a single patch? 

I think most clang-tidy patches come as a single patch. When the patch is too large people will usually tell you to split it up :)
As far as I know, folks working on clang-tidy prefer a single patch, but for the static analyzer smaller patches are preferred. But indeed they will let you know.


On Tue, Oct 16, 2018 at 2:29 AM Manuel Klimek <[hidden email]> wrote:
On Tue, Oct 16, 2018 at 4:14 AM Shyan Akmal <[hidden email]> wrote:
Hello,

I'm working with a team of students that's interested in contributing to the clang front end (with the initial step of writing new checks for clang-tidy). 

We'd like to setup a workflow that makes it easy for us to review each other's work before officially submitting changes upstream, and also will be amenable to officially integrating our changes to clang-tidy.

My advice would be to not do that, but to do all reviews publicly in phabricator. Otherwise the reviewer that accepts the patch will potentially go into all the same questions again that were discussed earlier, and the later you get architectural feedback, the more expensive to change it, so the chance to get insight from folks as early as possible is a huge benefit.

Generally, I'd want to teach students that one of the most important rules of software development is that change is cheaper the earlier it is made, so discovering what needs to change as early as possible is probably the single most important skill one can develop early in one's career :)
 
Our current plan is to fork the git mirror of LLVM, and make commits to this copy of the repo on GitHub. After we finish writing and reviewing a check, we'd get the diff and submit a patch through Phabricator. 

This seems inline with the process suggested in the LLVM docs (from https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-git-svn and https://llvm.org/docs/Phabricator.html). 

Questions
  • Does this workflow make sense, and is it in accordance with LLVM developer policy?
  • Is there an alternate (or more standard) workflow that anyone thinks would work better?
  • If we work through a git repo, is it necessary to use git-svn to make sure svn (and not just git) has changes saved? 
  • Are there any specific practices we should adopt to make sure our additions can be successfully integrated into the current version of LLVM?
Best,
Shyan

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about Workflow & Submitting Patches

via cfe-dev
Ah alright, makes sense. Thanks again everyone for the help!

On Wed, Oct 17, 2018 at 1:43 AM Kristóf Umann <[hidden email]> wrote:


On 17 Oct 2018 10:09, "Manuel Klimek via cfe-dev" <[hidden email]> wrote:
On Wed, Oct 17, 2018 at 1:41 AM Shyan Akmal <[hidden email]> wrote:
Thank you everyone for the suggestions! The details in the workflow suggested by Whisperity seem like they'll be very helpful to us moving forward. 

Manuel's point of keeping all reviews public makes a lot of sense. Are there any general guidelines on how large we can make a single patch? In general I know that keeping changes smaller is better, but is it fine to write an entirely new clang-tidy check, and then submit that as a single patch? 

I think most clang-tidy patches come as a single patch. When the patch is too large people will usually tell you to split it up :)
As far as I know, folks working on clang-tidy prefer a single patch, but for the static analyzer smaller patches are preferred. But indeed they will let you know.


On Tue, Oct 16, 2018 at 2:29 AM Manuel Klimek <[hidden email]> wrote:
On Tue, Oct 16, 2018 at 4:14 AM Shyan Akmal <[hidden email]> wrote:
Hello,

I'm working with a team of students that's interested in contributing to the clang front end (with the initial step of writing new checks for clang-tidy). 

We'd like to setup a workflow that makes it easy for us to review each other's work before officially submitting changes upstream, and also will be amenable to officially integrating our changes to clang-tidy.

My advice would be to not do that, but to do all reviews publicly in phabricator. Otherwise the reviewer that accepts the patch will potentially go into all the same questions again that were discussed earlier, and the later you get architectural feedback, the more expensive to change it, so the chance to get insight from folks as early as possible is a huge benefit.

Generally, I'd want to teach students that one of the most important rules of software development is that change is cheaper the earlier it is made, so discovering what needs to change as early as possible is probably the single most important skill one can develop early in one's career :)
 
Our current plan is to fork the git mirror of LLVM, and make commits to this copy of the repo on GitHub. After we finish writing and reviewing a check, we'd get the diff and submit a patch through Phabricator. 

This seems inline with the process suggested in the LLVM docs (from https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-git-svn and https://llvm.org/docs/Phabricator.html). 

Questions
  • Does this workflow make sense, and is it in accordance with LLVM developer policy?
  • Is there an alternate (or more standard) workflow that anyone thinks would work better?
  • If we work through a git repo, is it necessary to use git-svn to make sure svn (and not just git) has changes saved? 
  • Are there any specific practices we should adopt to make sure our additions can be successfully integrated into the current version of LLVM?
Best,
Shyan

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev