Hello again,
so, we have Jenkins that runs build tests on our master branch. That makes working together on a huge project much easier. However, do we have a rule that all the code should be build tested? and if not, should we establish one?
I've recently seen how much trouble it can cause when a whole new platform directory isn't hooked up for build tests. One has to double check every patch touching a file there. If in doubt, even run manual tests. That's a huge burden not only during development but also for reviewers. A burden that Jenkins would be happy to take if we let him. And even if people try to take care, they're just people, the untested code will break eventually.
Of course, there'll always be a gap when a new platform is added. We could make it a rule, though, that no commit should be merged to the master branch, before the one that hooks the build test up is reviewed. Then, if anything goes wrong, and it's still not hooked up after 24h, revert it.
How about that?
Nico
Nico Huber via coreboot coreboot@coreboot.org schrieb am So., 26. Jan. 2020, 02:07:
Hello again,
so, we have Jenkins that runs build tests on our master branch. That makes working together on a huge project much easier. However, do we have a rule that all the code should be build tested? and if not, should we establish one?
The general rule of thumb is to build test everything in the tree. We're not doing that in a few cases, among them stuff behind default-off kconfig flags, but I think we should strive for improvement.
I've recently seen how much trouble it can cause when a whole new platform directory isn't hooked up for build tests. One has to double check every patch touching a file there. If in doubt, even run manual tests. That's a huge burden not only during development but also for reviewers. A burden that Jenkins would be happy to take if we let him. And even if people try to take care, they're just people, the untested code will break eventually.
Of course, there'll always be a gap when a new platform is added. We could make it a rule, though, that no commit should be merged to the master branch, before the one that hooks the build test up is reviewed.
This means that such code isn't build tested for even longer, effectively the whole development cycle until all the ducks are lined up.
Then, if anything goes wrong, and it's still not hooked up after 24h,
revert it.
How about that?
How about this: we allow utterly incomplete mainboards in, if they're hidden behind some flag. Abuild and Jenkins build these, regular users don't even see them.
Maybe even in a designated area within src/mainboard? "Staging" perhaps?
We have a number of conflicting goals (having usable code presented to users, getting development into the main tree early to prevent code drops after everything is said and done, incremental changes to ease review and following what's going on) and that would provide a reasonable compromise: the unfinished code is clearly identified as such but it's there for (automated) testing and we can encourage incremental, incomplete changes there.
Regards, Patrick
Hi Patrick,
On 26.01.20 10:48, Patrick Georgi wrote:
Nico Huber via coreboot coreboot@coreboot.org schrieb am So., 26. Jan. 2020, 02:07:
I've recently seen how much trouble it can cause when a whole new platform directory isn't hooked up for build tests. One has to double check every patch touching a file there. If in doubt, even run manual tests. That's a huge burden not only during development but also for reviewers. A burden that Jenkins would be happy to take if we let him. And even if people try to take care, they're just people, the untested code will break eventually.
Of course, there'll always be a gap when a new platform is added. We could make it a rule, though, that no commit should be merged to the master branch, before the one that hooks the build test up is reviewed.
This means that such code isn't build tested for even longer, effectively the whole development cycle until all the ducks are lined up.
you mean it's less tested when it's sitting in the review queue, waiting for the hook-up? I don't see any difference in that aspect. As long as it's not hooked up for Jenkins, only the developers that work on the new platform will do build tests. And they can only spot conflicts when/ after rebasing the patch train, no matter if parts of it are already merged. However, in case of conflicts, one could update patches that aren't merged yet. But for patches that are already merged, one would have to write fixups (more patches) and would have room to repeat the error (I feel like I've seen fixups for fixups before).
Then, if anything goes wrong, and it's still not hooked up after 24h,
revert it.
How about that?
How about this: we allow utterly incomplete mainboards in, if they're hidden behind some flag. Abuild and Jenkins build these, regular users don't even see them.
The "utterly" aside, that's already common practice. Just that the mainboards aren't hidden. And I don't see a problem with that. In case of new platforms, "users" aren't expected to have the boards anyway.
Maybe even in a designated area within src/mainboard? "Staging" perhaps?
Probably overkill, how about simply having a warning instead of the board name in the Kconfig prompt?
We have a number of conflicting goals (having usable code presented to users, getting development into the main tree early to prevent code drops after everything is said and done, incremental changes to ease review and following what's going on) and that would provide a reasonable compromise: the unfinished code is clearly identified as such but it's there for (automated) testing and we can encourage incremental, incomplete changes there.
Ack.
Nico
Of course, there'll always be a gap when a new platform is added. We could make it a rule, though, that no commit should be merged to the master branch, before the one that hooks the build test up is reviewed.
This means that such code isn't build tested for even longer, effectively the whole development cycle until all the ducks are lined up.
you mean it's less tested when it's sitting in the review queue, waiting for the hook-up? I don't see any difference in that aspect. As long as it's not hooked up for Jenkins, only the developers that work on the new platform will do build tests. And they can only spot conflicts when/ after rebasing the patch train, no matter if parts of it are already merged. However, in case of conflicts, one could update patches that aren't merged yet. But for patches that are already merged, one would have to write fixups (more patches) and would have room to repeat the error (I feel like I've seen fixups for fixups before).
For an individual developer this might make sense, but for large projects I think that will make automation and coordination across multiple companies unfeasible. The likely outcome is that large projects are developed internally and eventually we see a huge code drop after the product has been released (when the "ducks are lined up" as Patrick says), and by then nobody involved with development really cares about fixing up the code since their paycheck depends on launching the next thing.
Then the community moans about not having a voice earlier in the process and wonder why companies aren't doing development upstream. In other words we go back to 2012 when Chromebooks started appearing upstream (hundreds of patches at a time being pushed from an internal repo to upstream) or 2020 when massive coreboot patches based on an old commit get posted elsewhere (like https://github.com/teslamotors/coreboot).
Maybe even in a designated area within src/mainboard? "Staging" perhaps?
Probably overkill, how about simply having a warning instead of the board name in the Kconfig prompt?
Agreed, and moving code around also makes history more difficult to follow so it's best to get the code structure in place early on if possible.
We have a number of conflicting goals (having usable code presented to users, getting development into the main tree early to prevent code drops after everything is said and done, incremental changes to ease review and following what's going on) and that would provide a reasonable compromise: the unfinished code is clearly identified as such but it's there for (automated) testing and we can encourage incremental, incomplete changes there.
Ack.
+1. It's a difficult set of goals to balance, but we should certainly strive to improve the process for everyone.
Hi David,
I'm a bit confused by some of your arguments, it just seems weird. So my apologies in advance if I misinterpreted you.
On 26.01.20 19:46, David Hendricks wrote:
Of course, there'll always be a gap when a new platform is added. We could make it a rule, though, that no commit should be merged to the master branch, before the one that hooks the build test up is reviewed.
This means that such code isn't build tested for even longer, effectively the whole development cycle until all the ducks are lined up.
you mean it's less tested when it's sitting in the review queue, waiting for the hook-up? I don't see any difference in that aspect. As long as it's not hooked up for Jenkins, only the developers that work on the new platform will do build tests. And they can only spot conflicts when/ after rebasing the patch train, no matter if parts of it are already merged. However, in case of conflicts, one could update patches that aren't merged yet. But for patches that are already merged, one would have to write fixups (more patches) and would have room to repeat the error (I feel like I've seen fixups for fixups before).
For an individual developer this might make sense, but for large projects I think that will make automation and coordination across multiple companies unfeasible.
We are still talking about adding new platform support to coreboot and build testing the code, right? I try to always keep the whole process in mind. And all I've said so far was to make it possible to work better together on such an endeavor even across multiple companies and the coreboot community. I have no idea how you got the impression that my view is too narrow.
The likely outcome is that large projects are developed internally and eventually we see a huge code drop after the product has been released (when the "ducks are lined up" as Patrick says), and by then nobody involved with development really cares about fixing up the code since their paycheck depends on launching the next thing.
How? Why? I really miss a step in your argumentation. Is it like with the Underpants Gnomes?
1. Build tests 2. ... 3. Downstream development
What I proposed was to merge patches later. Not to review them later. Technically the only difference would be when is a commit copied to another branch. It's still the same commit, just on a different branch. So what's the difference for working together?
So I really don't see how that would push people away from Gerrit. I would even expect the exact opposite. If you merge patches that are not hooked up for build testing early, they are more likely to break. So what happens when somebody in one company rebases and has to fix a line somewhere; will they commit and push the patch imme- diately so everyone else working on the new platform can fetch it? I highly doubt that. However, if the queue stays on Gerrit, it would only cause build breakage when that queue is rebased and it would be much easier to share the resulting fixes.
But what am I talking about. David, please share experience and explain your workflow and argue why build tests would break it.
Then the community moans about not having a voice earlier in the process and wonder why companies aren't doing development upstream. In other words we go back to 2012 when Chromebooks started appearing upstream (hundreds of patches at a time being pushed from an internal repo to upstream) or 2020 when massive coreboot patches based on an old commit get posted elsewhere (like https://github.com/teslamotors/coreboot).
Please explain your argumentation before making wild claims about consequences. This reads like propaganda.
Maybe even in a designated area within src/mainboard? "Staging" perhaps?
Probably overkill, how about simply having a warning instead of the board name in the Kconfig prompt?
Agreed, and moving code around also makes history more difficult to follow so it's best to get the code structure in place early on if possible.
It's code, so it's always possible.
Nico
On 26.01.20 19:46, David Hendricks wrote:
Of course, there'll always be a gap when a new platform is added. We could make it a rule, though, that no commit should be merged to the master branch, before the one that hooks the build test up is reviewed.
This means that such code isn't build tested for even longer, effectively the whole development cycle until all the ducks are lined up.
you mean it's less tested when it's sitting in the review queue, waiting for the hook-up? I don't see any difference in that aspect. As long as it's not hooked up for Jenkins, only the developers that work on the new platform will do build tests. And they can only spot conflicts when/ after rebasing the patch train, no matter if parts of it are already merged. However, in case of conflicts, one could update patches that aren't merged yet. But for patches that are already merged, one would have to write fixups (more patches) and would have room to repeat the error (I feel like I've seen fixups for fixups before).
For an individual developer this might make sense, but for large projects I think that will make automation and coordination across multiple companies unfeasible.
We are still talking about adding new platform support to coreboot and build testing the code, right? I try to always keep the whole process in mind. And all I've said so far was to make it possible to work better together on such an endeavor even across multiple companies and the coreboot community. I have no idea how you got the impression that my view is too narrow.
I just don't think the model you propose is realistic. IMO the best case is that it results in huge patch chains that churn a lot and are frustrating to work with. And in the worst (and most likely) case it will result in new silicon and platforms being developed out upstream entirely.
Is it like with the Underpants Gnomes?
Speaking of weird arguments...
So what happens when somebody in one company rebases and has to fix a line somewhere; will they commit and push the patch imme- diately so everyone else working on the new platform can fetch it? I highly doubt that.
That's the current expectation - pull from master, rebase, and push fixes when needed.
However, if the queue stays on Gerrit, it would only cause build breakage when that queue is rebased and it would be much easier to share the resulting fixes.
I think this model will result in huge patch chains on gerrit, and in my experience it's never easy to shuffle patches and fixes that way.
I simply disagree with you here, and based on the reaction of others in the community it appears I'm not alone in thinking that your proposed workflow is unfeasible.
But what am I talking about. David, please share experience and explain your workflow and argue why build tests would break it.
Then the community moans about not having a voice earlier in the process and wonder why companies aren't doing development upstream. In other words we go back to 2012 when Chromebooks started appearing upstream (hundreds of patches at a time being pushed from an internal repo to upstream) or 2020 when massive coreboot patches based on an old commit get posted elsewhere (like https://github.com/teslamotors/coreboot).
Please explain your argumentation before making wild claims about consequences. This reads like propaganda.
Here's ya go: https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/message/6YV7...
To put it another way, coreboot needs upstream development under the current model more than it needs your idealistic (and unrealistic) development model. Not that things in the current model are perfect, but I'd rather see the AMD code get merged in an imperfect state, developed toward maturity, and eventually hooked into the build system rather than have it developed out of tree.
Hi David,
I'm not so sure what we argue about here. The hypothetical case that it's hard to hook things up for build testing early, right? I've haven't seen that yet.
On 26.01.20 23:49, David Hendricks wrote:
On 26.01.20 19:46, David Hendricks wrote:
Of course, there'll always be a gap when a new platform is added. We could make it a rule, though, that no commit should be merged to the master branch, before the one that hooks the build test up is reviewed.
This means that such code isn't build tested for even longer, effectively the whole development cycle until all the ducks are lined up.
you mean it's less tested when it's sitting in the review queue, waiting for the hook-up? I don't see any difference in that aspect. As long as it's not hooked up for Jenkins, only the developers that work on the new platform will do build tests. And they can only spot conflicts when/ after rebasing the patch train, no matter if parts of it are already merged. However, in case of conflicts, one could update patches that aren't merged yet. But for patches that are already merged, one would have to write fixups (more patches) and would have room to repeat the error (I feel like I've seen fixups for fixups before).
For an individual developer this might make sense, but for large projects I think that will make automation and coordination across multiple companies unfeasible.
We are still talking about adding new platform support to coreboot and build testing the code, right? I try to always keep the whole process in mind. And all I've said so far was to make it possible to work better together on such an endeavor even across multiple companies and the coreboot community. I have no idea how you got the impression that my view is too narrow.
I just don't think the model you propose is realistic. IMO the best case is that it results in huge patch chains that churn a lot and are frustrating to work with. And in the worst (and most likely) case it will result in new silicon and platforms being developed out upstream entirely.
Again, what's the difference if half of the train is already merged (but rotting, remember we are talking about code that is very fragile because not build tested and the project is big)?
So what happens when somebody in one company rebases and has to fix a line somewhere; will they commit and push the patch imme- diately so everyone else working on the new platform can fetch it? I highly doubt that.
That's the current expectation - pull from master, rebase, and push fixes when needed.
Yes, but I'm convinced it's more work to fix commits that are merged already.
However, if the queue stays on Gerrit, it would only cause build breakage when that queue is rebased and it would be much easier to share the resulting fixes.
I think this model will result in huge patch chains on gerrit, and in my experience it's never easy to shuffle patches and fixes that way.
I simply disagree with you here, and based on the reaction of others in the community it appears I'm not alone in thinking that your proposed workflow is unfeasible.
Then please, propose another workflow (that scales in a large project like coreboot, without getting on the wrong side of others).
But what am I talking about. David, please share experience and explain your workflow and argue why build tests would break it.
Then the community moans about not having a voice earlier in the process and wonder why companies aren't doing development upstream. In other words we go back to 2012 when Chromebooks started appearing upstream (hundreds of patches at a time being pushed from an internal repo to upstream) or 2020 when massive coreboot patches based on an old commit get posted elsewhere (like https://github.com/teslamotors/coreboot).
Please explain your argumentation before making wild claims about consequences. This reads like propaganda.
Here's ya go: https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/message/6YV7...
To put it another way, coreboot needs upstream development under the current model more than it needs your idealistic (and unrealistic) development model. Not that things in the current model are perfect, but I'd rather see the AMD code get merged in an imperfect state, developed toward maturity, and eventually hooked into the build system rather than have it developed out of tree.
If you want to discuss trouble to get patches reviewed, please open another thread. This one is about build testing. To make it easier to work together on a big project while avoiding collisions.
Nico
I'm not so sure what we argue about here. The hypothetical case that it's hard to hook things up for build testing early, right? I've haven't seen that yet.
Let's step back for a moment. The proposal as I understand is that all code that lands in the tree must be hooked up so it's built by Jenkins, correct?
That sounds good at first, but comes with a lot of ramifications. One example, if I'm understanding your proposal, is that one can no longer commit some initial SOC support followed up some time later by a mainboard commit that uses it. Instead enough will need to be finished in order to be buildable, but until then things linger on Gerrit.
Please correct me if I'm way off base with that example. The stubs Patrick proposed in the other thread might help address the issue, however it can also mean adding code which exists only to satisfy the build requirement, churns as the real code changes, and may need to be removed later on anyway.
If you want to discuss trouble to get patches reviewed, please open another thread. This one is about build testing. To make it easier to work together on a big project while avoiding collisions.
You wanted some clarity on my "wild claims about consequences". The recent drama shows us that when development on coreboot.org becomes a PITA developers can just take their work elsewhere (or private). Which development model works for a particular project (or sub-project within coreboot) depends on a lot of variables and I don't think we can expect many people to stick around if we make unfeasible demands of their workflow.
Am Di., 28. Jan. 2020 um 08:03 Uhr schrieb David Hendricks < david.hendricks@gmail.com>:
Please correct me if I'm way off base with that example. The stubs Patrick proposed in the other thread might help address the issue, however it can also mean adding code which exists only to satisfy the build requirement, churns as the real code changes, and may need to be removed later on anyway.
Right, but it means that there's less of a risk of API changes across the tree (that happen all the time) throwing bring-up projects off-track. I mean, these stubs don't have to _do_ anything useful, they merely have to present themselves as just good enough to the build system to put all source code through the grinder^Wcompiler.
I'd expect all developers to have _some_ kind of board set up for their SoC because otherwise how do they test the SoC code in the first place? (I assume it is tested, at least to some degree) Maybe it's good enough to push these early, potentially anonymized plus some signifier that they're stubs? Not sure if that's practical in all existing workflows, but I'll leave that to a separate thread.
I suppose we could look into having jenkins throw out a warning if a source file (*.c or *.h) wasn't touched during the build. That might be a good exercise in any case to see what the situation looks like right now.
Regards, Patrick
-
David Hendricks -
Nico Huber -
Patrick Georgi