Allowing PRs on GitHub for some subprojects

Project Infrastructure LLVM Dev List Archives
21

I know there has been significant discussion about “moving” from
Phabricator to GitHub reviews and pull requests, etc. I’m not
suggesting that we do anything in terms of global LLVM policy.
However, as a maintainer of libc++, I commit a lot of other
people’s code for them. It would be a huge time saver for me if I
could nicely suggest to contributors (not force them) to use PRs
instead of Phabricator for their contributions. It would also handle
commit attribution properly, which is a pain right now.

Don’t take this as me telling you it is “actually simple”. I am
interested what about the contribution is problematic? If the libc++
system doesn’t have more requirements than the rest of LLVM there might
be ways to make it less painful. FWIW, here is what I do, and I know not
everyone wants to use arc. Ina script this could potentially reduce
the pain. Again, this is not meant to tell you it is simple or your
problems are not real.

arc patch DXXXX
git pull --rebase origin master
arc amend
arcfilter // see below
git llvm push master

arcfilter () { git log -1 --pretty=%B | awk ‘/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:/’ | git commit --amend -F - }

Thanks, this indeed solves some of my problems, however not entirely. When people submit contributions without an email address, I still have to do some digging to find out how to attribute the change. This shouldn’t be something I even have to think about.

I usually ask the patch author in the review how they want the attribution.

22

Mehdi, Chris & others,

I guess I did not express the main reasons for wanting to switch over very well in my original message.

You original message was about “ commit attribution”, but now it is all about testing?

Instead of jumping to a solution (pull-request) why not expressing the actual problem (lack of pre merge testing) and discuss it and explore all the possible solutions?
I think the discussion would be much more productive if we take it from first principles here.

Like Christian talked about, for me it’s all about pre-commit testing. I believe pre-commit testing is a widely shared desire among this community. However, how badly it is missed depends on sub-projects, because they have different realities. For example, in libc++:

  1. We have a lot of first-time contributors, which means that the maintainers end up shepherding many contributions. In particular, this often means fixing small breakage following their changes, which can be difficult for them because they can’t reproduce the failures locally, and they might not even know where to look. While these contributors can submit valuable improvements and bug fixes, we can’t expect them to fix every last platform that we support in the current state of things – it’s hard, it’s boring, and it’s stressful.

  2. Our testing matrix is very large, and interactions between different configurations (usually #ifs/#elses) is very subtle. This means the rate of mistake-on-first-try is, I think, higher in libc++ than in most other LLVM projects. Even with careful review, I find that a large percentage of changes end up breaking something somewhere, and I have to fix it (usually quickly enough to avoid reverting).

As a result, the lack of pre-commit testing is actively harming the health of libc++ as a project. It might be true for other projects as well, but I can only speak for libc++ because that’s where I have first hand experience.

What changed recently that makes this suddenly critical compared to the previous years?

Unfortunately, we currently don’t have a good way of doing pre-commit testing on Phabricator AFAICT

I thought we do now? I got a bunch of libcxx failing on my revision a few weeks ago.
The LLVM-premerge-test project recently added presubmit on Linux.

Windows will hopefully follow soon (in beta right now I believe), and Mac afterward! (Even though mac is lacking on the cloud provider availability)

. From the Harbormaster documentation [1]:

You’ll need to write a nontrivial amount of code to get this working today. In the future, Harbormaster will become more powerful and have more builtin support for interacting with build systems.

So while I appreciate all the efforts being made in this area, I still don’t even know where to start if I want to setup pre-commit testing for libc++ today. However, the path is very clear with GitHub PRs and there are many options available.

Whenever I hear arguments of dividing the community, not being able to share infrastructure, the lack of Herald – those all make a lot of sense to me and I think they’re good arguments.

However, it is clear that folks who even think about these arguments are not paying the same cost for the lack of good pre-commit testing that I’m paying on a weekly basis,

FYI that can read quite condescending…

23

Mehdi, Chris & others,

I guess I did not express the main reasons for wanting to switch over very well in my original message.

You original message was about “ commit attribution”, but now it is all about testing?

It was about allowing GitHub PRs for libc++, and one of the reasons I cited was commit attribution.

Instead of jumping to a solution (pull-request) why not expressing the actual problem (lack of pre merge testing) and discuss it and explore all the possible solutions?
I think the discussion would be much more productive if we take it from first principles here.

That’s really what I’ve tried doing below. Quoting myself:

And if the solution is that Harbormaster suddenly becomes usable without an unreasonable time investment from me, then I’m fine with that too. I’m not looking to switch to GitHub PRs for the sake of it, I’m looking to solve problems that are harming libc++ in the current system.

Maybe I should have stated that earlier on.

Like Christian talked about, for me it’s all about pre-commit testing. I believe pre-commit testing is a widely shared desire among this community. However, how badly it is missed depends on sub-projects, because they have different realities. For example, in libc++:

  1. We have a lot of first-time contributors, which means that the maintainers end up shepherding many contributions. In particular, this often means fixing small breakage following their changes, which can be difficult for them because they can’t reproduce the failures locally, and they might not even know where to look. While these contributors can submit valuable improvements and bug fixes, we can’t expect them to fix every last platform that we support in the current state of things – it’s hard, it’s boring, and it’s stressful.

  2. Our testing matrix is very large, and interactions between different configurations (usually #ifs/#elses) is very subtle. This means the rate of mistake-on-first-try is, I think, higher in libc++ than in most other LLVM projects. Even with careful review, I find that a large percentage of changes end up breaking something somewhere, and I have to fix it (usually quickly enough to avoid reverting).

As a result, the lack of pre-commit testing is actively harming the health of libc++ as a project. It might be true for other projects as well, but I can only speak for libc++ because that’s where I have first hand experience.

What changed recently that makes this suddenly critical compared to the previous years?

At the risk of oversharing, this has been my biggest source of pain and frustration since I’ve became a libc++ maintainer. It’s always been a problem since I started about 2 years ago. I can still feel the hurt of fixing the ripples of landing aligned allocation in libc++ in 2018, which lasted for weeks and weeks (on and off).

Last week, I spent about 4 days fixing the ripples of a high-quality contribution that happened to break some bot configurations (and some botless ones too). That was both very stressful and frustrating, and I swore to myself this was going to be the last week like that.

Unfortunately, we currently don’t have a good way of doing pre-commit testing on Phabricator AFAICT

I thought we do now? I got a bunch of libcxx failing on my revision a few weeks ago.

My understanding is that Christian was the one to setup the pre-merge testing we have currently (thanks!). And he says:

This is soooooo much easier to set up and maintain than the Phabricator integration

Furthermore, what libc++ needs in this area is not necessarily the same as the rest of LLVM. I don’t know what pre-merge bots are setup right now, and it’s certainly better than nothing, but we have a lot of test configurations that just don’t really make sense for a toolchain:

  • test suite in c++03/c++11/c++14/etc
  • exceptions on/off
  • with per-TU guarantee enabled/disabled
  • all the macOS back-deployment targets against system dylibs
  • etc.

I’m not expecting anyone else to set these up cause it doesn’t make sense. I need to be empowered to do it myself, and right now I’m not (or I’m really not looking in the right spot).

The LLVM-premerge-test project recently added presubmit on Linux.

Windows will hopefully follow soon (in beta right now I believe), and Mac afterward! (Even though mac is lacking on the cloud provider availability)

. From the Harbormaster documentation [1]:

You’ll need to write a nontrivial amount of code to get this working today. In the future, Harbormaster will become more powerful and have more builtin support for interacting with build systems.

So while I appreciate all the efforts being made in this area, I still don’t even know where to start if I want to setup pre-commit testing for libc++ today. However, the path is very clear with GitHub PRs and there are many options available.

Whenever I hear arguments of dividing the community, not being able to share infrastructure, the lack of Herald – those all make a lot of sense to me and I think they’re good arguments.

However, it is clear that folks who even think about these arguments are not paying the same cost for the lack of good pre-commit testing that I’m paying on a weekly basis,

FYI that can read quite condescending…

Sorry, that was really not my intent. The point I tried to make is that we have different priorities because we work on different projects, which have different requirements. So what may seem like just an annoyance to some might be a huge quality of life and productivity issue for someone else.

Louis

24

Mehdi, Chris & others,

I guess I did not express the main reasons for wanting to switch over very well in my original message.

You original message was about “ commit attribution”, but now it is all about testing?

It was about allowing GitHub PRs for libc++, and one of the reasons I cited was commit attribution.

Instead of jumping to a solution (pull-request) why not expressing the actual problem (lack of pre merge testing) and discuss it and explore all the possible solutions?
I think the discussion would be much more productive if we take it from first principles here.

That’s really what I’ve tried doing below. Quoting myself:

And if the solution is that Harbormaster suddenly becomes usable without an unreasonable time investment from me, then I’m fine with that too. I’m not looking to switch to GitHub PRs for the sake of it, I’m looking to solve problems that are harming libc++ in the current system.

Maybe I should have stated that earlier on.

Like Christian talked about, for me it’s all about pre-commit testing. I believe pre-commit testing is a widely shared desire among this community. However, how badly it is missed depends on sub-projects, because they have different realities. For example, in libc++:

  1. We have a lot of first-time contributors, which means that the maintainers end up shepherding many contributions. In particular, this often means fixing small breakage following their changes, which can be difficult for them because they can’t reproduce the failures locally, and they might not even know where to look. While these contributors can submit valuable improvements and bug fixes, we can’t expect them to fix every last platform that we support in the current state of things – it’s hard, it’s boring, and it’s stressful.

  2. Our testing matrix is very large, and interactions between different configurations (usually #ifs/#elses) is very subtle. This means the rate of mistake-on-first-try is, I think, higher in libc++ than in most other LLVM projects. Even with careful review, I find that a large percentage of changes end up breaking something somewhere, and I have to fix it (usually quickly enough to avoid reverting).

As a result, the lack of pre-commit testing is actively harming the health of libc++ as a project. It might be true for other projects as well, but I can only speak for libc++ because that’s where I have first hand experience.

What changed recently that makes this suddenly critical compared to the previous years?

At the risk of oversharing, this has been my biggest source of pain and frustration since I’ve became a libc++ maintainer. It’s always been a problem since I started about 2 years ago. I can still feel the hurt of fixing the ripples of landing aligned allocation in libc++ in 2018, which lasted for weeks and weeks (on and off).

Last week, I spent about 4 days fixing the ripples of a high-quality contribution that happened to break some bot configurations (and some botless ones too). That was both very stressful and frustrating, and I swore to myself this was going to be the last week like that.

Unfortunately, we currently don’t have a good way of doing pre-commit testing on Phabricator AFAICT

I thought we do now? I got a bunch of libcxx failing on my revision a few weeks ago.

My understanding is that Christian was the one to setup the pre-merge testing we have currently (thanks!). And he says:

This is soooooo much easier to set up and maintain than the Phabricator integration

Of course it is easier to do on GitHub, but the integration on Phabricator can also a one-time cost for the project, which is already kind of on the way with https://github.com/google/llvm-premerge-checks ; then it is a matter of making it easier to add your own bot the pre-merge and increase the coverage.

Furthermore, what libc++ needs in this area is not necessarily the same as the rest of LLVM. I don’t know what pre-merge bots are setup right now, and it’s certainly better than nothing, but we have a lot of test configurations that just don’t really make sense for a toolchain:

  • test suite in c++03/c++11/c++14/etc
  • exceptions on/off
  • with per-TU guarantee enabled/disabled
  • all the macOS back-deployment targets against system dylibs
  • etc.

I just see this as different configurations / builders that you have to provide if you care about, just like our current post-merge buildbot infrastructure actually: http://lab.llvm.org:8011/buildslaves
For example MLIR would want to run test on machines with GPUs (to test our CUDA and Vulkan paths), which are of no interest to most of the other projects, yet that seems fairly orthogonal to the mechanism to register a new bots / config with the system.

25

Mehdi, Chris & others,

I guess I did not express the main reasons for wanting to switch over very well in my original message.

You original message was about “ commit attribution”, but now it is all about testing?

It was about allowing GitHub PRs for libc++, and one of the reasons I cited was commit attribution.

Instead of jumping to a solution (pull-request) why not expressing the actual problem (lack of pre merge testing) and discuss it and explore all the possible solutions?
I think the discussion would be much more productive if we take it from first principles here.

That's really what I've tried doing below. Quoting myself:

> And if the solution is that Harbormaster suddenly becomes usable without an unreasonable time investment from me, then I'm fine with that too. I'm not looking to switch to GitHub PRs for the sake of it, I'm looking to solve problems that are harming libc++ in the current system.

Maybe I should have stated that earlier on.

Like Christian talked about, for me it's all about pre-commit testing. I believe pre-commit testing is a widely shared desire among this community. However, how badly it is missed depends on sub-projects, because they have different realities. For example, in libc++:

1. We have a lot of first-time contributors, which means that the maintainers end up shepherding many contributions. In particular, this often means fixing small breakage following their changes, which can be difficult for them because they can't reproduce the failures locally, and they might not even know where to look. While these contributors can submit valuable improvements and bug fixes, we can't expect them to fix every last platform that we support in the current state of things -- it's hard, it's boring, and it's stressful.

2. Our testing matrix is very large, and interactions between different configurations (usually #ifs/#elses) is very subtle. This means the rate of mistake-on-first-try is, I think, higher in libc++ than in most other LLVM projects. Even with careful review, I find that a large percentage of changes end up breaking something somewhere, and I have to fix it (usually quickly enough to avoid reverting).

As a result, the lack of pre-commit testing is actively harming the health of libc++ as a project. It might be true for other projects as well, but I can only speak for libc++ because that's where I have first hand experience.

What changed recently that makes this suddenly critical compared to the previous years?

At the risk of oversharing, this has been my biggest source of pain and frustration since I've became a libc++ maintainer. It's always been a problem since I started about 2 years ago. I can still feel the hurt of fixing the ripples of landing aligned allocation in libc++ in 2018, which lasted for weeks and weeks (on and off).

Last week, I spent about 4 days fixing the ripples of a high-quality contribution that happened to break some bot configurations (and some botless ones too). That was both very stressful and frustrating, and I swore to myself this was going to be the last week like that.

Unfortunately, we currently don't have a good way of doing pre-commit testing on Phabricator AFAICT

I thought we do now? I got a bunch of libcxx failing on my revision a few weeks ago.

My understanding is that Christian was the one to setup the pre-merge testing we have currently (thanks!). And he says:

   > This is soooooo much easier to set up and maintain than the Phabricator integration

Furthermore, what libc++ needs in this area is not necessarily the same as the rest of LLVM. I don't know what pre-merge bots are setup right now, and it's certainly better than nothing, but we have a lot of test configurations that just don't really make sense for a toolchain:
- test suite in c++03/c++11/c++14/etc
- exceptions on/off
- with per-TU guarantee enabled/disabled
- all the macOS back-deployment targets against system dylibs
- etc.

I'm not expecting anyone else to set these up cause it doesn't make sense. I need to be empowered to do it myself, and right now I'm not (or I'm really not looking in the right spot).

Looking at the few commits Louis made today:

commit a27f29c6e498266fa8d8d38c3c3f74305c684f8d
commit 2b2a1a42c0a24f5a98ce1161e8ea100552126644
commit 30cbdcb5c3694e2e6c4647ce88df9e3692bf90cf

They perfectly demonstrate that libc++ indeed requires a wider range of
configurations that the existing pre-merge testing instructure cannot
satisfy :confused:

I think libc++abi and libunwind may be in the same boat.
These subprojects just have lots of variety.

26

I am not sure if you’re being sarcastic here? From what I see here this is the kind of things that just requires building with a specific version of the compiler, and all of them seems even testable on Linux (not even some specific MSVC for fun?).
Isn’t this trivially testable in a Docker config on any cloud machine? If so there is no real infrastructure complexity I really see and the pre-merge infrastructure can perfectly suit this as well as any other infra as far as I can tell.

Compare to for example: bootstrapping a compiler and using it to cross-compile the test-suite that you need then to run on actual embedded HW devices, or any use case where you can’t use the cloud and have to maintain your own lab with exotic hardware (looking forward to have an FPGA codegen in MLIR and see the kind of runtime test environment we’d need…).

Best,

27

Mehdi, Chris & others,

I guess I did not express the main reasons for wanting to switch over very well in my original message.

You original message was about “ commit attribution”, but now it is all about testing?

It was about allowing GitHub PRs for libc++, and one of the reasons I cited was commit attribution.

Instead of jumping to a solution (pull-request) why not expressing the actual problem (lack of pre merge testing) and discuss it and explore all the possible solutions?
I think the discussion would be much more productive if we take it from first principles here.

That’s really what I’ve tried doing below. Quoting myself:

And if the solution is that Harbormaster suddenly becomes usable without an unreasonable time investment from me, then I’m fine with that too. I’m not looking to switch to GitHub PRs for the sake of it, I’m looking to solve problems that are harming libc++ in the current system.

Maybe I should have stated that earlier on.

Like Christian talked about, for me it’s all about pre-commit testing. I believe pre-commit testing is a widely shared desire among this community. However, how badly it is missed depends on sub-projects, because they have different realities. For example, in libc++:

  1. We have a lot of first-time contributors, which means that the maintainers end up shepherding many contributions. In particular, this often means fixing small breakage following their changes, which can be difficult for them because they can’t reproduce the failures locally, and they might not even know where to look. While these contributors can submit valuable improvements and bug fixes, we can’t expect them to fix every last platform that we support in the current state of things – it’s hard, it’s boring, and it’s stressful.

  2. Our testing matrix is very large, and interactions between different configurations (usually #ifs/#elses) is very subtle. This means the rate of mistake-on-first-try is, I think, higher in libc++ than in most other LLVM projects. Even with careful review, I find that a large percentage of changes end up breaking something somewhere, and I have to fix it (usually quickly enough to avoid reverting).

As a result, the lack of pre-commit testing is actively harming the health of libc++ as a project. It might be true for other projects as well, but I can only speak for libc++ because that’s where I have first hand experience.

What changed recently that makes this suddenly critical compared to the previous years?

At the risk of oversharing, this has been my biggest source of pain and frustration since I’ve became a libc++ maintainer. It’s always been a problem since I started about 2 years ago. I can still feel the hurt of fixing the ripples of landing aligned allocation in libc++ in 2018, which lasted for weeks and weeks (on and off).

Last week, I spent about 4 days fixing the ripples of a high-quality contribution that happened to break some bot configurations (and some botless ones too). That was both very stressful and frustrating, and I swore to myself this was going to be the last week like that.

Unfortunately, we currently don’t have a good way of doing pre-commit testing on Phabricator AFAICT

I thought we do now? I got a bunch of libcxx failing on my revision a few weeks ago.

My understanding is that Christian was the one to setup the pre-merge testing we have currently (thanks!). And he says:

This is soooooo much easier to set up and maintain than the Phabricator integration

Furthermore, what libc++ needs in this area is not necessarily the same as the rest of LLVM. I don’t know what pre-merge bots are setup right now, and it’s certainly better than nothing, but we have a lot of test configurations that just don’t really make sense for a toolchain:

  • test suite in c++03/c++11/c++14/etc
  • exceptions on/off
  • with per-TU guarantee enabled/disabled
  • all the macOS back-deployment targets against system dylibs
  • etc.

I’m not expecting anyone else to set these up cause it doesn’t make sense. I need to be empowered to do it myself, and right now I’m not (or I’m really not looking in the right spot).

Looking at the few commits Louis made today:

commit a27f29c6e498266fa8d8d38c3c3f74305c684f8d
commit 2b2a1a42c0a24f5a98ce1161e8ea100552126644
commit 30cbdcb5c3694e2e6c4647ce88df9e3692bf90cf

They perfectly demonstrate that libc++ indeed requires a wider range of
configurations that the existing pre-merge testing instructure cannot
satisfy :confused:

I am not sure if you’re being sarcastic here? From what I see here this is the kind of things that just requires building with a specific version of the compiler, and all of them seems even testable on Linux (not even some specific MSVC for fun?).
Isn’t this trivially testable in a Docker config on any cloud machine? If so there is no real infrastructure complexity I really see and the pre-merge infrastructure can perfectly suit this as well as any other infra as far as I can tell.

I think the point being made is that given the large number of configurations, it’s not reasonable to run this pre-merge testing locally (even though the failures above could be reproduced locally). There’s just too many bots for that to be practical (47 if we count BuildBot and GreenDragon). I’ve been doing a subset of that manually with my Mac and docker images, and it’s a huge time sink for little benefit.

I don’t think you’re suggesting to test locally, but instead you’re saying that some future pre-merge infrastructure on Phabricator could run these bots – right? If so, I think you’re right, cause at the end of the day these configurations don’t require anything very exotic, indeed. But saying that is playing ostrich, cause the problem is that I am not empowered to setup these bots in the first place, the barrier being very high.

Louis

28

Mehdi, Chris & others,

I guess I did not express the main reasons for wanting to switch over very well in my original message.

You original message was about “ commit attribution”, but now it is all about testing?

It was about allowing GitHub PRs for libc++, and one of the reasons I cited was commit attribution.

Instead of jumping to a solution (pull-request) why not expressing the actual problem (lack of pre merge testing) and discuss it and explore all the possible solutions?
I think the discussion would be much more productive if we take it from first principles here.

That’s really what I’ve tried doing below. Quoting myself:

And if the solution is that Harbormaster suddenly becomes usable without an unreasonable time investment from me, then I’m fine with that too. I’m not looking to switch to GitHub PRs for the sake of it, I’m looking to solve problems that are harming libc++ in the current system.

Maybe I should have stated that earlier on.

Like Christian talked about, for me it’s all about pre-commit testing. I believe pre-commit testing is a widely shared desire among this community. However, how badly it is missed depends on sub-projects, because they have different realities. For example, in libc++:

  1. We have a lot of first-time contributors, which means that the maintainers end up shepherding many contributions. In particular, this often means fixing small breakage following their changes, which can be difficult for them because they can’t reproduce the failures locally, and they might not even know where to look. While these contributors can submit valuable improvements and bug fixes, we can’t expect them to fix every last platform that we support in the current state of things – it’s hard, it’s boring, and it’s stressful.

  2. Our testing matrix is very large, and interactions between different configurations (usually #ifs/#elses) is very subtle. This means the rate of mistake-on-first-try is, I think, higher in libc++ than in most other LLVM projects. Even with careful review, I find that a large percentage of changes end up breaking something somewhere, and I have to fix it (usually quickly enough to avoid reverting).

As a result, the lack of pre-commit testing is actively harming the health of libc++ as a project. It might be true for other projects as well, but I can only speak for libc++ because that’s where I have first hand experience.

What changed recently that makes this suddenly critical compared to the previous years?

At the risk of oversharing, this has been my biggest source of pain and frustration since I’ve became a libc++ maintainer. It’s always been a problem since I started about 2 years ago. I can still feel the hurt of fixing the ripples of landing aligned allocation in libc++ in 2018, which lasted for weeks and weeks (on and off).

Last week, I spent about 4 days fixing the ripples of a high-quality contribution that happened to break some bot configurations (and some botless ones too). That was both very stressful and frustrating, and I swore to myself this was going to be the last week like that.

Unfortunately, we currently don’t have a good way of doing pre-commit testing on Phabricator AFAICT

I thought we do now? I got a bunch of libcxx failing on my revision a few weeks ago.

My understanding is that Christian was the one to setup the pre-merge testing we have currently (thanks!). And he says:

This is soooooo much easier to set up and maintain than the Phabricator integration

Of course it is easier to do on GitHub, but the integration on Phabricator can also a one-time cost for the project, which is already kind of on the way with https://github.com/google/llvm-premerge-checks ; then it is a matter of making it easier to add your own bot the pre-merge and increase the coverage.

You seem to be dismissing the cost of setting up bots as being something that has to be paid once. In my experience, however, this is actually a recurring cost because these bots need maintenance. And whenever we add a new bot, it’s also a problem if we are confronted to this complexity again.

But like I said before, I’m only here to solve my (libc++) problems. And right now, my problem is that I literally can’t figure out how to setup a pre-merge bot on Phabricator. Do you know how to do that? In all seriousness, can you show me how to set up even a simple bot that would run the most basic libc++ configuration? I’m sure I would be able (and certainly willing) to extrapolate from there and set up the other missing configurations without your help.

But if you can’t help me, then who can? Honestly, I think we’re all talking out of our hats and the only person who truly has gone through the process is Christian – and you’ve seen his email.

Cheers,
Louis

29

Mehdi, Chris & others,

I guess I did not express the main reasons for wanting to switch over very well in my original message.

You original message was about “ commit attribution”, but now it is all about testing?

Instead of jumping to a solution (pull-request) why not expressing the actual problem (lack of pre merge testing) and discuss it and explore all the possible solutions?
I think the discussion would be much more productive if we take it from first principles here.

The first principle I would like to work from is “Keeping libc++ bugfree while enabling development velocity.”.
Build-policing, maintaining infrastructure, and keeping haunted graveyards are jobs I wish to give up.

Like Christian talked about, for me it’s all about pre-commit testing. I believe pre-commit testing is a widely shared desire among this community. However, how badly it is missed depends on sub-projects, because they have different realities. For example, in libc++:

  1. We have a lot of first-time contributors, which means that the maintainers end up shepherding many contributions. In particular, this often means fixing small breakage following their changes, which can be difficult for them because they can’t reproduce the failures locally, and they might not even know where to look. While these contributors can submit valuable improvements and bug fixes, we can’t expect them to fix every last platform that we support in the current state of things – it’s hard, it’s boring, and it’s stressful.

  2. Our testing matrix is very large, and interactions between different configurations (usually #ifs/#elses) is very subtle. This means the rate of mistake-on-first-try is, I think, higher in libc++ than in most other LLVM projects. Even with careful review, I find that a large percentage of changes end up breaking something somewhere, and I have to fix it (usually quickly enough to avoid reverting).

As a result, the lack of pre-commit testing is actively harming the health of libc++ as a project. It might be true for other projects as well, but I can only speak for libc++ because that’s where I have first hand experience.

What changed recently that makes this suddenly critical compared to the previous years?

The number of new contributors, and the amount of development being done by other groups.

30

Mehdi, Chris & others,

I guess I did not express the main reasons for wanting to switch over very well in my original message.

You original message was about “ commit attribution”, but now it is all about testing?

It was about allowing GitHub PRs for libc++, and one of the reasons I cited was commit attribution.

Instead of jumping to a solution (pull-request) why not expressing the actual problem (lack of pre merge testing) and discuss it and explore all the possible solutions?
I think the discussion would be much more productive if we take it from first principles here.

That’s really what I’ve tried doing below. Quoting myself:

And if the solution is that Harbormaster suddenly becomes usable without an unreasonable time investment from me, then I’m fine with that too. I’m not looking to switch to GitHub PRs for the sake of it, I’m looking to solve problems that are harming libc++ in the current system.

Maybe I should have stated that earlier on.

Like Christian talked about, for me it’s all about pre-commit testing. I believe pre-commit testing is a widely shared desire among this community. However, how badly it is missed depends on sub-projects, because they have different realities. For example, in libc++:

  1. We have a lot of first-time contributors, which means that the maintainers end up shepherding many contributions. In particular, this often means fixing small breakage following their changes, which can be difficult for them because they can’t reproduce the failures locally, and they might not even know where to look. While these contributors can submit valuable improvements and bug fixes, we can’t expect them to fix every last platform that we support in the current state of things – it’s hard, it’s boring, and it’s stressful.

  2. Our testing matrix is very large, and interactions between different configurations (usually #ifs/#elses) is very subtle. This means the rate of mistake-on-first-try is, I think, higher in libc++ than in most other LLVM projects. Even with careful review, I find that a large percentage of changes end up breaking something somewhere, and I have to fix it (usually quickly enough to avoid reverting).

As a result, the lack of pre-commit testing is actively harming the health of libc++ as a project. It might be true for other projects as well, but I can only speak for libc++ because that’s where I have first hand experience.

What changed recently that makes this suddenly critical compared to the previous years?

At the risk of oversharing, this has been my biggest source of pain and frustration since I’ve became a libc++ maintainer. It’s always been a problem since I started about 2 years ago. I can still feel the hurt of fixing the ripples of landing aligned allocation in libc++ in 2018, which lasted for weeks and weeks (on and off).

Last week, I spent about 4 days fixing the ripples of a high-quality contribution that happened to break some bot configurations (and some botless ones too). That was both very stressful and frustrating, and I swore to myself this was going to be the last week like that.

Unfortunately, we currently don’t have a good way of doing pre-commit testing on Phabricator AFAICT

I thought we do now? I got a bunch of libcxx failing on my revision a few weeks ago.

My understanding is that Christian was the one to setup the pre-merge testing we have currently (thanks!). And he says:

This is soooooo much easier to set up and maintain than the Phabricator integration

Of course it is easier to do on GitHub, but the integration on Phabricator can also a one-time cost for the project, which is already kind of on the way with https://github.com/google/llvm-premerge-checks ; then it is a matter of making it easier to add your own bot the pre-merge and increase the coverage.

You seem to be dismissing the cost of setting up bots as being something that has to be paid once.

Sorry if I wasn’t clear on this: I was referring to the difference between integrating a bot infrastructure with Phabricator vs GitHub as a one-time cost. The extra cost unique to Phabricator should be paid once for the project, adding more bots to a pre-merge system that is triggered by and report to Phab should then be fairly equivalent afterward hopefully.

In my experience, however, this is actually a recurring cost because these bots need maintenance. And whenever we add a new bot, it’s also a problem if we are confronted to this complexity again.

But like I said before, I’m only here to solve my (libc++) problems. And right now, my problem is that I literally can’t figure out how to setup a pre-merge bot on Phabricator. Do you know how to do that? In all seriousness, can you show me how to set up even a simple bot that would run the most basic libc++ configuration? I’m sure I would be able (and certainly willing) to extrapolate from there and set up the other missing configurations without your help.

But if you can’t help me, then who can? Honestly, I think we’re all talking out of our hats and the only person who truly has gone through the process is Christian – and you’ve seen his email.

I started working on this in September, anticipating the migration of MLIR into the monorepo, the only reason I stopped is because I learnt that Christian was doing it as well.
My understanding is that the goal of https://github.com/google/llvm-premerge-checks is not to setup a rigid one-time bot for Phabricator, but reach a point where adding a new configuration should have a playbook similar to https://llvm.org/docs/HowToAddABuilder.html

Best,

31

Hello all,

I’d like to share my thoughts on this topic. Most of my experience with LLVM is through libc++ so, that’s what I’ll talk about but, I assume that most of this applies to other sub-projects as well.

There are four things that I think everyone would agree are important to allow us to succeed in collaborating on a shared codebase. These things are:

  1. A good reviewing tool.
  2. Issues tracking.
  3. A good testing framework.
  4. The ability for the maximum number of people to contribute high-quality patches.

It’s hard to compare the review tool. Lots of people have strong opinions about it, and it’s mostly subjective.

We have already moved some sub-projects to GitHub issues. This means we have four different places to look for the “status” of something. We have the status page, GitHub issues, Bugzilla, and Phabricator/GitHub. To check if something has been implemented, a person has to look through an undefinable combination of those sites. Imagine this instead: the status of issues in the current implementation, LWG issues, and papers would all be tracked on GitHub. Those issues could be linked to a patch made on GitHub, and both the issue and the patch could be assigned to someone. Currently, to check the status of something, you have to look through all four of those platforms above and maybe ask on the mailing list to check if someone’s working on it already. If we use GitHub, we eliminate both the difficulty of checking and the semi-frequent occurrence of two people implementing the same thing.

Libc++ has a great set of unit tests but, they have lots of different (usually inline) configurations. Often (especially for newer contributors), all the tests are run in several configurations, and then the patch is committed. Later the buildbots find something, and the patch has to be reverted. This is unnecessary. If the buildbots were run before the patch lands, we could catch the majority of the errors early.

When a patch is submitted to libc++ by anyone except the three maintainers, a few things happen. First, it’s reviewed. That process works well and shouldn’t change too much. Second, it often needs to be tested by one of the maintainers. While I’m not a maintainer of libc++, I can imagine that this process can be time-consuming, especially considering the volume of patches submitted. Last, the change needs to be committed. This involves downloading the raw patch, applying it, and committing on behalf of the person. Then, watching the bots and sometimes reverting the patch. This is also time-consuming. On GitHub, we can configure the PRs with requirements. These requirements can be things like, “x number of maintainers approve this patch.” Or, “the following buildbots succeed.” These requirements mean that the reviewers are no longer responsible for committing, running the tests, or reverting. GitHub will make sure the tests pass before anything is committed, then after it’s been approved, the contributor can commit and revert the code themself (without commit access).

TL;DR: There are only three maintainers and their time is very valuable. Currently, we waste lots of it by having them manually go through a series of steps that are error-prone and time-consuming, but easily automatable. We should automate them such that they have a higher success rate and don’t waste valuable time.

Best,

Zoe

32

Hi Zoe,

I hear you and understand where you want to go. Asides from the Github PR/issues migration (which I am a big fan of):
I’ve been chatting with Louis and Eric on extending the pre-merge tests for libc++ to better support your use cases. I’d be happy to add additional checks there.

If it’s really urgent, we can try to add additional checks right away and trigger these when someone uploads a diff for /libcxx to Phabricator. We probably need to fiddle with the scripting and triggers to figure out which builds/tests to run for a diff, but that should be feasible.

33

Thanks for your message Zoe. It looks like we’re exactly on the same page w.r.t. where we want to go.

Louis