micro: add a mechanism for excluding test and kernel sources from Makefile build

[rkuester]

@tensorflow/micro

When submitting a change for review which both copies and modifies a file, it's helpful to submit the copy and the modification as two separate PRs. Otherwise the PR's files changed tab does not show the modifications, but rather it looks as if the copied and modified file was simply added in its final form. See, for example, the sequence of PRs in the flight plan of issue #45306.

To facilitate submitting a PR which adds a file which should not yet be a part of the build (for the copy PR), add to the micro Makefile a means to exclude kernel and test sources from the build.

[rkuester]

@petewarden @advaitjain, I've associated two competing PRs with this issue, two different ways of solving the problem. I'm concerned the exclusion lists will lead to merge conflicts. And it's a little more straightforward to have a file show up in the list once its ready to be built, rather than appearing and then disappearing from an exclusion list.

One last poke at whether copying and modifying a file must be in separate PRs:

What's lost if both steps are in the same PR but as two separate commits within the PR? I understand the Files changed tab shows the whole diff merged together, which makes it look as if the copied and modified file was simply added in its final form; however, that's simply the default view. Both the Changes from dropdown (under Conversation) and the Commits tab let's you see the commits individually, which makes it easy to see the modifications as distinct from the copying. This seems to achieve the goals cleanly without two separate PRs.

The only tradeoff I see to one, joint PR is that both commits must be merged or rejected as one unit; however, in this case, I don't see the value in merging only the copy and having it sit there in the directory as dead code. In fact, it seems preferable to have them show up together or not at all.

If the copy and modify can be in the same PR, this exclusion mechanism is less necessary, but still justified perhaps so the copy commit doesn't break the build (and therefore git bisect, etc.). The explicit-list alternative still looks better to me in that case than the exclusion-list alternative.

Thoughts?

[advaitjain]

I'm with you that explicitly listing the kernel sources is the path forward here -- let's go with that.

Why I'd like to keep two separate PRs:

• When PRs are imported internally, all the commits are squashed as a single change so to keep the copy and modify separate in the internal google version control system we need them to be separate PRs.

• Even in github, I view PRs as a unit of change (rather than commits) since these are where reviews happen, are properly documented ... The build is often broken at individual commits and the only guarantee is that the merge commit for a PR is where the build is ok. In an ideal world, I would like to have PRs be merged with a squash commit to keep the git log 1:1 with the PRs.

[rkuester]

I've rebased the chosen approach, PR #45389, and marked it ready for review.

Thanks for documenting your rational for keeping two separate PRs for copy and modify. Will do.

[rkuester]

With the merger of #45389, this closed.

[advaitjain]

One last poke at whether copying and modifying a file must be in separate PRs:

What's lost if both steps are in the same PR but as two separate commits within the PR? I understand the Files changed tab shows the whole diff merged together, which makes it look as if the copied and modified file was simply added in its final form; however, that's simply the default view. Both the Changes from dropdown (under Conversation) and the Commits tab let's you see the commits individually, which makes it easy to see the modifications as distinct from the copying. This seems to achieve the goals cleanly without two separate PRs.

The only tradeoff I see to one, joint PR is that both commits must be merged or rejected as one unit; however, in this case, I don't see the value in merging only the copy and having it sit there in the directory as dead code. In fact, it seems preferable to have them show up together or not at all.

If the copy and modify can be in the same PR, this exclusion mechanism is less necessary, but still justified perhaps so the copy commit doesn't break the build (and therefore git bisect, etc.). The explicit-list alternative still looks better to me in that case than the exclusion-list alternative.

Thoughts?

We revisited this discussion internally again and @njeffrie made exactly the same argument as you did here.

And after seeing the work involved in having the copy and modify be in separate PRs as well as the overhead of having dead code in the repo, I now agree with you guys.

Your proposed approach is much better. Let's go with having one commit each for:

1. copy the tflite kernel
2. remove lite-specific code from copies
3. port the micro op and test

This would mean that PR3-5 in your draft op porting guide (https://github.com/rkuester/tensorflow/blob/f06b3273eb2492ca9a93399a6c6cf1492b3fd569/tensorflow/lite/micro/kernels/PORTING.md) would become 3 commits in the same PR.

@njeffrie, @rkuester : does that match what you guys think would be a better process?

We can still keep the explicit list of kernel sources, even though we may not necessarily need them anymore.

[rkuester]

@njeffrie, @rkuester : does that match what you guys think would be a better process?

Yes, combining PRs 3–5 into one PR with at least those three commits makes a lot of sense to me. I'll update the guide.