Hi Angel, thanks for the links I will definitely check out those examples. In the meantime, I've discovered the concept of gerrit 'topics' so I'm using that currently to start split out the commits into logical related changes for easier review but still be able to build š. Right now, the majority of the refactor is happening in the topic 'Denverton Refactor', and is marked as WIP. Still learning gerrit, so if there's a way for you to find that, you'll see the basics of the work I'm doing. I think the topic method is really the only way I can see to break up things cleanly and still have it be able to build to get the +1. Right now I'm pushing them as the soc/intel/denverton_ns chunk, then the mainboard/intel/harcuvar chunk, then the mainboard/silicom/denverton_ns (the way I've structured our multiple boards, I may actually separate them out into individual boards, right now they are baseboard/variant mechanism since they are very similar but I might change my mind). If you have a suggestion that works better than topics I'm all ears, but for now I'm moving forward this way. If you get to look at the code (I'll see if I can just send an invite or something), be assured that I'll be breaking the commit up into digestible chunks so as you said, finding the ones that cause a problem will make it easier.
Also, yes I acknowledge that I need to take care of the Tagada board port to the new refactor, I'll probably be doing that this weekend. I did contact them but they response was that they've changed directions so the Tagada is unmaintained at this time (and sounds like it won't become a product).
Additionally, I've been in contact with my former coworkers at Intel. They are having a meeting on Tuesday to discuss the support moving forward, but in all likelihood they will pass on the maintenance to me (they've already asked me if I'd be willing in fact).
-----Original Message----- From: Angel Pons th3fanbus@gmail.com Sent: Saturday, January 8, 2022 8:43 AM To: Jeff Daly jeffd@silicom-usa.com Cc: Felix Singer felixsinger@posteo.net; coreboot@coreboot.org Subject: Re: [coreboot] Re: Denverton-NS refactoring
Caution: This is an external email. Please take care when clicking links or opening attachments.
Hi Jeff,
On Fri, Jan 7, 2022 at 5:13 AM Jeff Daly jeffd@silicom-usa.com wrote:
Another thing I'd like to say (and it's probably pretty obvious once the majority of the commits start coming through) is that a lot of work has been done for this. In order to make the commit chunks more logical in their grouping will require that the build will end up breaking because changes in one area will break another. I started by submitting the changes to the ifdtool because that's independent of the rest, followed by the changes to the southbridge/firmware area because again it won't break anything. Next, is the pci_ids.h because it's just letting everyone know that I'm changing the names globally (which of course will break the build). The majority of the change is soc/intel/denverton_ns so that commit will be a logical group, without worrying about platforms (again will break the build because the mainboards depend on it), and lastly I'll submit the mainboards/intel/harcuvar in order to have an example of a platform (and not break the build if all the other commits before it are part of the build.) since it look that the buildbot will try and build for each individual commit vs applying an entire set of commits and building, you'll either have to live with the breakage until everything is reviewed before applying everything all at once and fixing it again or someone can point me to an example of how doing a major overhaul like this would be usable. Normally I'd make a branch and do it that way, but it seems that's not the way to do it in this environment? Or did I miss something and I can actually create and push a branch of my own into this repo?
I'm afraid you'll have to restructure your work. Are these commits publicly visible somewhere? If not, you can push them to review.coreboot.org (just for reference, create new changes for the redesigned ). It's hard for me to give concrete suggestions without seeing the code, but I'll happily provide suggestions on how to restructure your work into upstreamable changes.
We don't do any merges with Gerrit, so every commit needs to build successfully to get submitted. This is very useful when doing a git bisect to troubleshoot a regression: if the faulty commit got submitted amidst the commits of a patch train that only builds successfully at the end, one would need to fix the build errors while bisecting, with the risk of introducing new bugs which would make it harder to identify where the original problem is. When a commit doesn't break building but results in runtime problems (e.g. boot failure), we call this a regression and we address regressions by either fixing the issue if it's easy to find (e.g. https://review.coreboot.org/55289 used the wrong variable and caused boot issues, https://review.coreboot.org/58558 fixed it) or by reverting the problematic commit.
Also, note that intel/harcuvar isn't the only Denverton-NS in the tree: scaleway/tagada would also need to be updated so that it builds successfully. In our workflow, changes to non-mainboard code that impact mainboard code also have to adapt mainboards accordingly. Yes, this is quite annoying when many boards are affected, but having to fix broken boards after non-mainboard code changes didn't update all affected boards accordingly is much worse by several orders of magnitude. Typically, the changes to mainboard code are only annoying because of their repetitive nature, but this shouldn't be an issue for Denverton-NS as there's only two boards in the tree. https://review.coreboot.org/49088 and https://review.coreboot.org/49089 are two examples of non-mainboard-code changes that affect mainboard code. Yes, I could've done this using only one commit, but I think it's easier to review the changes to mainboard code this way: the first commit just removes `cX_battery` settings under the premise that they have the same value as the corresponding `cX_acpower` settings, and the second commit just renames the `cX_acpower` settings to `acpi_cX`.
In most cases, it's best to make one commit per logical change, where intermediate states still work even if they're incomplete. When a commit message has a "list of changes" or similar, it's a sign that it can likely be split into several commits. If unsure, it's better to make too many commits and squash some of them later than to make too few commits and end up having to split them up. Intermediate states of my patch trains still work (unless I messed up), but can have ugly things that get fixed later. For example, https://review.coreboot.org/60937 doesn't unindent the body of the original if-block because it gets cleaned up later in https://review.coreboot.org/60938 which is reproducible. A commit is reproducible if the resulting coreboot.rom when building with BUILD_TIMELESS=1 (which avoids including some info like commit hash into the image) is identical before and after the commit, and verifying this is a great way to ensure that some commits are correct. I've (ab)used reproducible commits to make some changes to RAM init code much easier to review. RAM init code has lots of magic values, and changing just one number could have unpredictable consequences that regular boot testing can't detect. https://review.coreboot.org/q/hashtag:iosav-api is one example of such a thing, where I temporarily introduced an awfully offensive C macro with about two dozen parameters to wrap the code around the magic values so that changing how those magic values are used doesn't touch the magic values themselves. Even with the cursed macro in there, the code still built and booted successfully.
-----Original Message----- From: Felix Singer felixsinger@posteo.net Sent: Wednesday, January 5, 2022 9:31 PM To: coreboot@coreboot.org Subject: [coreboot] Re: Denverton-NS refactoring
Caution: This is an external email. Please take care when clicking links or opening attachments.
Hello Jeff,
that sounds really great. Thanks!
Not sure if the maintainers from Intel are still active or not, but Iām sending them an email to see.
They aren't. The Intel "maintainers" told us they will work on Denverton, while we were about to drop the Denverton support like a year ago, but nothing happened since then.
So your patches are highly appreciated :)
wondering what might be the best way to submit the refactor for easier review.
Hard to tell when we don't know what exactly has changed. I would say just push whatever you have to Gerrit and then we will see. They can be still rebased and reworked later :)
I suggest adding a topic to your patches (like "denverton_refactoring"), if some aren't in the same relation chain. So they are easier to find through the search.
// Felix
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Best regards, Angel