LWN: Comments on "LZ4: vendoring in the kernel"

LZ4: vendoring in the kernel

tterribe — Mon, 06 Feb 2017 03:57:11 +0000

We use the approach karkhaz describes in Firefox, also, with some amount of success: each vendored library has an update.sh script which copies the relevant files from a checkout of the relevant upstream version and applies all local patches (usually named after bug numbers that describe the issue and give steps to reproduce). If run, it should produce exactly what is in the tree.

Primarily, it serves as visible documentation so someone doesn't have to remember to run 'git log' on a bunch of files before patching/replacing them. The fact that you have to update it when you patch the vendored code is also a good reminder that someone else (like a future you) will have to deal with that patch on the next code import, and gives good incentive to move patches upstream if possible.

LZ4: vendoring in the kernel

iabervon — Thu, 02 Feb 2017 17:32:37 +0000

Patches that fix security problems in the context of the kernel but aren't appropriate in userspace sound extremely fragile: what happens when the upstream project adds another function that the kernel would have to change?

Seems like it would be worthwhile for the upstream project to have a header file with all of the bits of functions a user like the kernel might need to change, and the kernel could compile with a kernel-specific header file instead.

LZ4: vendoring in the kernel

mathstuf — Thu, 02 Feb 2017 16:43:40 +0000

We use something like git subtree at work, but not exactly (we evaluated subtree, but found it lacking for what we needed). What we do is keep a git mirror of the repo on our infra to which we add a for/$project branch. On this branch, we hold patches for integration into the build, fixes, symbol mangling patches, etc. Patches which should go upstream get issues opened for doing so.

To import, we take a subset of that repo (we usually do not care about docs and the like), put it in a tree, make a commit with its parent pointing to the previous commit (initial imports use a new root commit), and then merge into place using -Xsubtree. This allows us to keep the history as one would expect as well as not inflating our repo size with the full history of the import. Git checks ensure that the imported directory is only changed via this mechanism (and also protects against "evil merges").

LZ4: vendoring in the kernel

daurnimator — Thu, 02 Feb 2017 14:56:09 +0000

The benefit of using git subtrees is that you get commit-by-commit history on both sides:
- you'd be able to extract the kernel commits that applied to the LZ4 library, and send a PR upstream (or allow them to cherry pick).
- when pulling down changes, you get to maintain original commit messages (possibly e.g. mentioning CVEs) and dates as well as author attribution

LZ4: vendoring in the kernel

epa — Thu, 02 Feb 2017 14:51:26 +0000

Git subtree sounds good, but in fact you don't need anything too fancy to make sure you correctly track and reapply local patches.

Make a branch at the point where LZ4 code was first copied to the kernel. Working on that branch, upgrade the LZ4 code to the latest release. Then when you merge the branch back in, conflict resolution will automatically notice the locally applied changes to LZ4 in the meantime, either patching them in or flagging them as conflicts. As a final check you can diff the resulting LZ4 code against the vanilla LZ4 latest version to make sure the local patches still make sense.

LZ4: vendoring in the kernel

daurnimator — Thu, 02 Feb 2017 13:57:50 +0000

I'm wondering if anyone has tried to extract and merge the changes via 'git subtree'.
I don't know if the kernel does it already: but if you vendor code from a git-based project into a git-based project, **please** use 'git subtree' to do it.

LZ4: vendoring in the kernel

gregkh — Thu, 02 Feb 2017 10:07:49 +0000

The changes were commits to the source code, the kernel tree does not keep patches that need to be applied when the build happens.

You can see them easily by running 'git log' on the specific files you are curious about.

LZ4: vendoring in the kernel

karkhaz — Thu, 02 Feb 2017 09:51:38 +0000

> number of patches is small. Happily, they exist as separate patches, rather than having been quietly folded into the source when LZ4 was initially added to the kernel.

To clarify, are these "separate patches" a bunch of separate commits, or do they exist as individual patch files somewhere in the tree? I had a quick look through the tree but didn't find any patch files.

If they are separate commits, then indeed GKH's concern is valid, one must remember to rebase those commits back onto tip-of-tree every time the vendored code is updated. But there's a better way: if all the patches applied by the kernel are kept as separate files, and the `patch' command is used _as part of the build process, during every build_, then nobody has to remember anything. You keep the vendored code vanilla, and each patch is kept separately, and every time a patch gets upstreamed you remove it from the tree and update the vendored code to match.

This is how linux distros typically do things. See e.g. Arch Linux's source package for the kernel itself [0]. When building the package from source, the PKGBUILD file contains instructions on how to download the (vanilla) kernel, and also contains invocations to `patch' to correctly apply those (Arch Linux specific) patch files that you see in the directory. If I want to compile the kernel with _my own_ patches in addition to the Arch ones (so that it still works nicely on my Arch box) then I simply add one more patch file and run the build again.

[0] https://git.archlinux.org/svntogit/packages.git/tree/trun...

LZ4: vendoring in the kernel

iq-0 — Thu, 02 Feb 2017 08:13:34 +0000

Some explicit rule, like: "For any patches applied to vendored source, there must be at least a regression test added."?
(Along with another rule: "Any files that are vendored from external projects should include an explicit comment about it's origin, version and a reference to the regression test rule for patches.")

LZ4: vendoring in the kernel

klindsay — Thu, 02 Feb 2017 02:08:01 +0000

I'm not a kernel developer, so take the following comment from the peanut gallery with a grain of salt..

When I see "job that has to be remembered every time ... to avoid regressions ...", I can't help but think that a test suite for the in-kernel version of the LZ4 code would be an appropriate approach to deal with this.