build: Check every commit can be built

[sboeuf]

It'd be nice if we could have our CI verifying that each and every commit from a PR can be properly built. The main reason for that is to be able to run a git bisect if later in the future we're trying to understand where something might have gone wrong.

[sboeuf]

/cc @stillson

[glg-rv]

Not sure how I'd feel about that. At this moment the reason at least I use to separate in different commit is to ease the reviewing. I am okay with getting it to a build binary, but I wouldn't like the full blown lint + format. This is especially true when adding new features, where the introduction of those feature would temporarily have dead code.

[sboeuf]

Not sure how I'd feel about that. At this moment the reason at least I use to separate in different commit is to ease the reviewing. I am okay with getting it to a build binary, but I wouldn't like the full blown lint + format. This is especially true when adding new features, where the introduction of those feature would temporarily have dead code.

Per my experience, being able to bisect is a great benefit. I'm not at all suggesting we should not separate commits, this is quite important to keep patches as small as possible to give the reviewers an easy way to go through code reviews. Let's say you update a function with a new parameter, you would need to update the caller to make sure it provides the new parameter, that's for sure, but you can defer to another commit the actual change that will leverage this new parameter. On the dead code side of things, it's usually good to introduce unused code with #[allow(dead_code)] in the first commit introducing some new functions, and then remove this label when the following commit starts using these newly defined functions.

[glg-rv]

Any bisect script I have used would check if the commit builds, if not go up or down, depending on direction. Especially in a system that's based on CI on PRs you know that there's a commit that works and that is where you should bisect anyway, because it's the PR that possibly introduced the bug as whole, not a single commit.

Forcing every single commit to be both lint+format I feel would be an unnecessary slow down on development efforts, and complicating a developer life when you have to fix up patches before sending, while doing the pre-PR work.

I repeat, I am fine with requiring every single commit to build. But lint+format for each of them is just a big (am paraphrasing) "I hate you" to a programmer.

[abrestic-rivos]

I am very much in favor of this change, and it's also the norm in "production" projects (think Linux, QEMU, etc). Beyond the ability to bisect, it means that you can theoretically revert part of a series to get back to a clean state in the event something is broken. And it encourages good commit hygiene.

The main downside I see here is that CI is now O(n), but I don't think our runs take that long.

Anyways, will let others chime in.

[mnissler-rivos]

+1 to requiring that every commit builds and meets the quality bar we've set on all other dimensions as well (including lint+format, making lint+format changes subsequently ties commits together unnecessarily and effectively means you can only test/revert at pull request level).

That said, if we wanted to have smaller units for review, then squashing before submit could be an option as well (where it makes sense, we certainly don't want to bunch together unrelated/independent changes).

[glg-rv]

I strongly think that given rust's extreme insight by the standard tools we use in the code, using the full lint+format for code that is meant to be temporary is extreme and mostly a waste of development time.

[stillson]

We could probably just do build check on the intermediate steps, w/o lint and format checks.

Which would mean the code has to be cleaned up in we split up a commit.

We should probably hold each commit to the same standards.

[abrestic-rivos]

code that is meant to be temporary

Sure, if one considers the PR to be a quantum of code change then intra-PR changes could be considered temporary. But I think most would consider that to be the commit since that's the level at which git operates.

[glg-rv]

As strong as my opinion are, am clearly in the minority here. I can work with this, just not particularly enthusiastic about it.

[dgreid]

I'd be happy to see CI run on every commit in the name of bisectability. But I think it's rather challenging with github's CI system.

[sboeuf]

This has been taken care of.