-
Notifications
You must be signed in to change notification settings - Fork 334
OOT-related updates #1242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OOT-related updates #1242
Conversation
joe-lawrence
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good and I like the SRCDIR distinctions.
kpatch-build/kpatch-build
Outdated
| else | ||
| KPATCH_BUILD="/lib/modules/$ARCHVERSION/build" | ||
| fi | ||
| KPATCH_BUILD="$SRCDIR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed in favor of changing the below export KPATCH_BUILD="$KPATCH_BUILD" to export KPATCH_BUILD="$SRCDIR".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
| CPUS="$(getconf _NPROCESSORS_ONLN)" | ||
| CACHEDIR="${CACHEDIR:-$HOME/.kpatch}" | ||
| SRCDIR="$CACHEDIR/src" | ||
| KERNEL_SRCDIR="$CACHEDIR/src" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This SRCDIR/KERNEL_SRCDIR split is unfortunate, and is most likely going to be a future source of bugs (but I don't have any better ideas).
At the very least, a comment would help to describe the difference between the two for future code readers trying to make sense of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we rename the leftover SRCDIR to BUILDDIR? I think it will better convey it's purpose and then KERNEL_SRCDIR/OOT_MODULE_SRCDIR are easier to understand as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's much better. Then there would be no comment needed since the variables are more self-descriptive.
Previously OOT-module builds used `--sourcedir` to specify oot-module source directory which was a bit confusing and also denied users ability to build kpatches agains oot modules built against non-distro kernels. This patch adds `-p/--oot-module-src` option to specify source dir for oot module while keeping `--sourcedir` for kernel source directory specification. I also tried to disambiguate `SRCDIR` in kpatch-build a bit. Now there are 3 variables: - `KERNEL_SRCDIR` - contains path to kernel source directory - `OOT_MODULEL_SRCDIR` - contains path to out-of-tree module source directory - `BUILDDIR` - can be set to either of the above and is used for patch-related actions Another attempt at this was done by @omatiusha in dynup#1234 Signed-off-by: Artem Savkov <asavkov@redhat.com>
kpatch-build currently initializes KBUILD_EXTRA_SYMBOLS to "", denying users ability to set it themselves. Fixes: dynup#1238 Signed-off-by: Artem Savkov <asavkov@redhat.com>
|
v2: |
jpoimboe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, the variable rename helped a lot.
This does introduce user-visible breakage due to the changing behavior of --sourcedir with OOT modules. We should try to remember to bump the .y version in the next release.
This has two commits: one dealing with building OOT modules with non-distro kernels, this also includes some refactoring disambiguating
SRCDIRsince we now have 2 SRCDIRS - one for kernel and another one for oot-module. This superceeds #1234.Second commit tries to deal with #1238.