Ok, so what I've figured out is that it seems gerrit doesn't like it when the code gets refactored. I have several commits that are showing merge conflicts but the files that have conflicts are because a lot of code changes between them as things get refactored. Entire blocks of code in some files are removed, some files are just completely replaced with newer files that have the correct content in them. I'd hate to have to do something dumb like rename the files that have too many changes for gerrit/git to figure out. CB:61015 is a perfect example. Soc/intel/denverton_ns/soc_util.c was changed to delete functions from colliding with others in the SoC common code (or were just plain unnecessary). The change deletes get_tseg_memory(), get_top_of_low_memory(), get_top_of_upper_memory(). No other changes other than deletions and I'm getting a merge conflict. Either I'm dumb or something else is dumb. (and I have been dumb before, so maybe it's me but this one seems kinda obvious unless there's some setting someplace that complains when there's more than a few lines difference).
-----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