I am using 'drm-fixes' branch of Dave Airlie's kernel tree right now. Currently, it installs itself with a little strange kernel version string: $ uname -r 2.6.35-rc2-pomiocik+ Thus, the modules are installed in /lib/modules/2.6.35-rc2-pomiocik+. The problem is that linux-mod.eclass seems to strip the trailing '+' and install modules in /lib/modules/2.6.35-rc2-pomiocik. The environment in ${T} shows that KV_FULL lacks the trailing '+' indeed. I'm attaching build log and environment file of media-video/stk11xx as an example.
Created attachment 235115 [details] Build log of stk11xx
Created attachment 235117 [details] stk11xx build environment
Portage 2.2_rc67_p171 (default/linux/amd64/10.0/desktop, gcc-4.4.4, glibc-2.11.2-r0, 2.6.35-rc2-pomiocik+ x86_64) ================================================================= System uname: Linux-2.6.35-rc2-pomiocik+-x86_64-Intel-R-_Pentium-R-_Dual_CPU_T2330_@_1.60GHz-with-gentoo-2.0.1 Timestamp of tree: Sat, 12 Jun 2010 19:00:01 +0000 app-shells/bash: 4.1_p7 dev-java/java-config: 2.1.11 dev-lang/python: 2.6.5-r2, 2.7_pre20100606, 3.1.2-r3, 3.2_pre20100606 dev-util/cmake: 2.8.1-r2 sys-apps/baselayout: 2.0.1 sys-apps/openrc: 9999 sys-apps/sandbox: 2.2 sys-devel/autoconf: 2.13, 2.65 sys-devel/automake: 1.9.6-r3, 1.10.3, 1.11.1 sys-devel/binutils: 2.20.1-r1 sys-devel/gcc: 4.4.4 sys-devel/gcc-config: 1.4.1 sys-devel/libtool: 2.2.10 virtual/os-headers: 2.6.34 ACCEPT_KEYWORDS="amd64 ~amd64" ACCEPT_LICENSE="* -@EULA" CBUILD="x86_64-pc-linux-gnu" CFLAGS="-march=core2 -O2 -pipe" CHOST="x86_64-pc-linux-gnu" CONFIG_PROTECT="/etc /usr/share/X11/xkb" CONFIG_PROTECT_MASK="/etc/ca-certificates.conf /etc/env.d /etc/env.d/java/ /etc/fonts/fonts.conf /etc/gconf /etc/gentoo-release /etc/revdep-rebuild /etc/sandbox.d /etc/terminfo /etc/texmf/language.dat.d /etc/texmf/language.def.d /etc/texmf/updmap.d /etc/texmf/web2c" CXXFLAGS="-march=core2 -O2 -pipe" DISTDIR="/usr/local/portage/distfiles" EMERGE_DEFAULT_OPTS="--ask --keep-going" FEATURES="assume-digests collision-protect distlocks fixpackages news parallel-fetch preserve-libs protect-owned sandbox sfperms strict unmerge-logs unmerge-orphans userfetch userpriv usersandbox usersync" GENTOO_MIRRORS="http://distfiles.gentoo.org" LANG="pl_PL.UTF-8" LC_ALL="pl_PL.UTF-8" LDFLAGS="-Wl,-O1 -Wl,--as-needed -Wl,--hash-style=gnu" LINGUAS="" MAKEOPTS="-j3" PKGDIR="/usr/local/portage/packages" PORTAGE_CONFIGROOT="/" PORTAGE_RSYNC_OPTS="--recursive --links --safe-links --times --compress --force --whole-file --delete --stats --timeout=45 --exclude=/distfiles --exclude=/packages --exclude=/local" PORTAGE_TMPDIR="/tmp" PORTDIR="/usr/portage" PORTDIR_OVERLAY="/usr/portage/local/gamerlay /usr/portage/local/games /usr/portage/local/java-overlay /usr/portage/local/kde-sunset /usr/portage/local/qting-edge /usr/portage/local/x11 /usr/portage/local/perl-experimental /usr/portage/local/python /usr/portage/local/sunrise /usr/portage/local/science /usr/portage/local/gnome /home/mgorny/git/emdzientoo" SYNC="rsync://rsync.gentoo.org/gentoo-portage" USE="X a52 aac acl acpi alsa amd64 avahi bash-completion bluetooth branding bzip2 cairo cdr cli consolekit cracklib crypt cups curl cxx dbus dirac directfb djvu dri dts dvd dvdr emboss encode exif expat fam fbcon fftw firefox flac fontconfig fortran gd gdbm gif gmp gnome-keyring gnutls gpm graphviz gtk iconv icu idn imagemagick ipv6 java6 jbig jpeg jpeg2k kate lapack lcms libedit libnotify libproxy mad matroska mikmod mmap mmx mmxext mng modules mp3 mp4 mpeg mtp mudflap multilib ncurses nptl nptlonly ogg opencore-amr openexr opengl openmp pam pango pch pcre pdf perl png ppds pppd python qt3support qt4 raw readline reflection schroedinger sdl session slang smp sndfile speex spell spl sse sse2 ssl ssse3 startup-notification svg sysfs tcpd theora threads tiff timidity truetype unicode usb v4l2 vcd vim-syntax vorbis wavpack wifi wmf x264 xcb xcomposite xml xorg xulrunner xv xvid zlib" ALSA_CARDS="ali5451 als4000 atiixp atiixp-modem bt87x ca0106 cmipci emu10k1x ens1370 ens1371 es1938 es1968 fm801 hda-intel intel8x0 intel8x0m maestro3 trident usb-audio via82xx via82xx-modem ymfpci" ALSA_PCM_PLUGINS="adpcm alaw asym copy dmix dshare dsnoop empty extplug file hooks iec958 ioplug ladspa lfloat linear meter mmap_emul mulaw multi null plug rate route share shm softvol" APACHE2_MODULES="actions alias auth_basic authn_alias authn_anon authn_dbm authn_default authn_file authz_dbm authz_default authz_groupfile authz_host authz_owner authz_user autoindex cache dav dav_fs dav_lock deflate dir disk_cache env expires ext_filter file_cache filter headers include info log_config logio mem_cache mime mime_magic negotiation rewrite setenvif speling status unique_id userdir usertrack vhost_alias" ELIBC="glibc" INPUT_DEVICES="evdev synaptics" KERNEL="linux" LCD_DEVICES="bayrad cfontz cfontz633 glk hd44780 lb216 lcdm001 mtxorb ncurses text" RUBY_TARGETS="ruby18" SANE_BACKENDS="artec_eplus48u" USERLAND="GNU" VIDEO_CARDS="fbdev r300 r600 radeon vesa" XTABLES_ADDONS="quota2 psd pknock lscan length2 ipv4options ipset ipp2p iface geoip fuzzy condition tee tarpit sysrq steal rawnat logmark ipmark dhcpmac delude chaos account" Unset: CPPFLAGS, CTARGET, FFLAGS, INSTALL_MASK, PORTAGE_COMPRESS, PORTAGE_COMPRESS_FLAGS, PORTAGE_RSYNC_EXTRA_OPTS
I've took a closer look at it, and the version extraction method seems to be responsible. The Makefile gets the kernel release string like that: KERNELRELEASE = $(shell cat include/config/kernel.release 2> /dev/null) where the mentioned file contains: $ cat /usr/src/linux/include/config/kernel.release 2.6.35-rc2-pomiocik+ and the variables read by linux-info indeed do not specify the trailing '+'.
Created attachment 235171 [details] The kernel Makefile Line 948 explains how the '+' is added as localver-extra var. It is explained a while before: # For kernels without CONFIG_LOCALVERSION_AUTO compiled from an SCM that has # been revised beyond a tagged commit, `+' is appended to the version string # when not overridden by using "make LOCALVERSION=". This indicates that the # kernel is not a vanilla release version and has been modified.
As of 2.6.35-rc4, the linux-info.eclass is totally broken, installing modules to /lib/modules/2.6.35-rc4Error:/. This is because the semantics of the setlocalversion script have changed. Instead of trying to compute the version string in the eclass, it should use the version string which is computed by kbuild, and stored in the output directory in the file: include/config/kernel.release.
(In reply to comment #6) > Instead of trying to compute the version string in the eclass, it should use > the version string which is computed by kbuild, and stored in the output > directory in the file: include/config/kernel.release. For reference, the file .../include/config/kernel.release was first present somewhere around 2.6.18 (commit f1d28fb043b325dad8944647a52b20287e59d8a1); previously it was named .../.kernelrelease The file was introduced around 2.6.16 (commit cb58455c48dc43536e5548bdba4e916b2f0cf13d) These should be used if present, perhaps with the existing code remaining as a fallback for older kernels. Of course, the oldest 2.6 kernel in portage is 2.6.16...
1. kernel.release is ONLY available after 'make prepare' has been run. Prior to that, it might not exist, or might be wrong. 2. The makefile may fail to give output in many cases. I'm working on trying to ask the makefile what the localversion string is.
(In reply to comment #8) > 1. kernel.release is ONLY available after 'make prepare' has been run. Prior > to that, it might not exist, or might be wrong. I was under the impression that the eclass already required 'make prepare' to have been run, anyway. If it's wrong, this seems like straight up PEBKAC, and is not significantly different from the /usr/src/linux symlink being wrong, or KBUILD_OUTPUT being wrong. > 2. The makefile may fail to give output in many cases. Assume the build tree and source trees are not completely hosed, and that the make implementation is not completely broken. The prepare target depends on kernel.release, so 'make prepare' will either generate it, or fail with an error message.
(In reply to comment #9) > I was under the impression that the eclass already required 'make prepare' to > have been run, anyway. If it's wrong, this seems like straight up PEBKAC, and > is not significantly different from the /usr/src/linux symlink being wrong, or > KBUILD_OUTPUT being wrong. This assumption is only valid when BOTH linux-mod and linux-info are being used together. > Assume the build tree and source trees are not completely hosed, and that the > make implementation is not completely broken. The prepare target depends on > kernel.release, so 'make prepare' will either generate it, or fail with an > error message. You're assuming there IS a build tree. The most common case usage of linux-info isn't building modules, it's checking for config options under which a program might not work. Early in a system install, the kernel-sources may have been installed (due to a dependency), but they have not been configured yet. This used to cause emerge to fail, and was explicitly changed, so that absence of sources, or their configuration was non-fatal unless the sources were actually required during the build process (almost exclusively for kernel modules).
I need a tester for something, ideally somebody that gets the + in their local version. Can you open up linux-info.eclass, and remove line 551 "linux_chkconfig_builtin LOCALVERSION_AUTO &&", so that the line after it always runs?
(In reply to comment #10) > (In reply to comment #9) > > I was under the impression that the eclass already required 'make prepare' > > to have been run, anyway. If it's wrong, this seems like straight up > > PEBKAC, and is not significantly different from the /usr/src/linux symlink > > being wrong, or KBUILD_OUTPUT being wrong. > This assumption is only valid when BOTH linux-mod and linux-ijkGnfo are being > used together. Ah, I see. I was focusing on the "modules don't get installed to the right place" part of the issue. > > Assume the build tree and source trees are not completely hosed, and that > > the make implementation is not completely broken. The prepare target > > depends on kernel.release, so 'make prepare' will either generate it, or > > fail with an error message. > You're assuming there IS a build tree. The most common case usage of > linux-info isn't building modules, it's checking for config options under > which a program might not work. Just to throw it out there: how many of these users require actually require KV_FULL? Could we get by with defining KV_FULL iff kernel.release is available? Does any ebuild actually use KV_LOCAL? Can we punt it? Manually computing the version string for purposes of determining module install locations just seems like a bad idea prone to fail next time things get changed.
(In reply to comment #10) > You're assuming there IS a build tree. The most common case usage of linux-info > isn't building modules, it's checking for config options under which a program > might not work. But does that require getting the version already? (In reply to comment #11) > I need a tester for something, ideally somebody that gets the + in their local > version. > > Can you open up linux-info.eclass, and remove line 551 "linux_chkconfig_builtin > LOCALVERSION_AUTO &&", so that the line after it always runs? Now the module got installed in /lib/modules/2.6.35-rc3-pomiocik-10729-g4ba9649 (while the correct name is still 2.6.35-rc3-pomiocik+).
What output do you get from just running script/setlocalversion? It should be '+' when CONFIG_LOCALVERSION_AUTO is NOT 'y'.
(In reply to comment #13) > But does that require getting the version already? Yes, it does. Specifically to populate the version variables, so ebuilds can make decisions about what config settings to ask for (since they get renamed sometimes).
(In reply to comment #14) > What output do you get from just running script/setlocalversion? > It should be '+' when CONFIG_LOCALVERSION_AUTO is NOT 'y'. $ scripts/setlocalversion -10729-g4ba9649 I suggest you to take a look at scripts in current git sources. (In reply to comment #15) > (In reply to comment #13) > > But does that require getting the version already? > Yes, it does. Specifically to populate the version variables, so ebuilds can > make decisions about what config settings to ask for (since they get renamed > sometimes). Local version part too? Maybe we'd just get the components the applications really need and move modulepath guessing to linux-mod.eclass.
(In reply to comment #16) > (In reply to comment #14) > > What output do you get from just running script/setlocalversion? > > It should be '+' when CONFIG_LOCALVERSION_AUTO is NOT 'y'. > > $ scripts/setlocalversion > -10729-g4ba9649 > > I suggest you to take a look at scripts in current git sources. I've got a tree as of a few hours ago, running that on this box. Ah, maybe the problem source is in how it's detecting CONFIG_LOCALVERSION_AUTO. # scm version string if not at a tagged commit if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then # full scm version string res="$res$(scm_version)" else # apped a plus sign if the repository is not in a clean tagged # state and LOCALVERSION= is not specified if test "${LOCALVERSION+set}" != "set"; then scm=$(scm_version --short) res="$res${scm:++}" fi fi > > (In reply to comment #15) > > (In reply to comment #13) > > > But does that require getting the version already? > > Yes, it does. Specifically to populate the version variables, so ebuilds can > > make decisions about what config settings to ask for (since they get renamed > > sometimes). > > Local version part too? Maybe we'd just get the components the applications > really need and move modulepath guessing to linux-mod.eclass. Maybe.
scripts/setlocalversion is not meant to be executed outside the kernel source tree. See line 138: if test -e include/config/auto.conf; then source "$_" else echo "Error: kernelrelease not valid - run 'make prepare' to update it" exit 1 fi but in linux-info.eclass there is KV_LOCAL="${KV_LOCAL}$(sh ${KV_DIR}/scripts/setlocalversion ${KV_DIR})" at line 552.
Looks like that script is actually inconsistent. It does take a directory argument, and does use that argument for SOME things, but not all. Let's change line 552: KV_LOCAL="${KV_LOCAL}$(cd ${KV_DIR} ; sh ${KV_DIR}/scripts/setlocalversion ${KV_DIR})" tell me if that gets you the '+' you're expecting? I still only get the scm version data, not a '+'.
(In reply to comment #19) > Let's change line 552: > KV_LOCAL="${KV_LOCAL}$(cd ${KV_DIR} ; sh ${KV_DIR}/scripts/setlocalversion > ${KV_DIR})" > > tell me if that gets you the '+' you're expecting? I still only get the scm > version data, not a '+'. The '+' isn't generated by the setlocalversion script; it's added by the makefile (which ignores the setlocalversion output in this case) when CONFIG_LOCALVERSION_AUTO is not set. The Makefile comments specify the exact conditions. The section of the Makefile that actually generates the version number starts at line 885 in current git.
(In reply to comment #20) > The '+' isn't generated by the setlocalversion script; it's added by the > makefile (which ignores the setlocalversion output in this case) when > CONFIG_LOCALVERSION_AUTO is not set. The Makefile comments specify the exact > conditions. The section of the Makefile that actually generates the version > number starts at line 885 in current git. I'm sorry, but you're wrong. The + is clearly generated by the setlocalversion script in the latest tree (e467e104bb7 right now). The Makefile _always_ runs setlocalversion, and there is no other means for the + to be set. Makefile: # Store (new) KERNELRELASE string in include/config/kernel.release include/config/kernel.release: include/config/auto.conf FORCE $(Q)rm -f $@ $(Q)echo "$(KERNELVERSION)$$($(CONFIG_SHELL) $(srctree)/scripts/setlocalversion $(srctree))" > $@ scripts/setlocalversion: ... # scm version string if not at a tagged commit if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then # full scm version string res="$res$(scm_version)" else # apped a plus sign if the repository is not in a clean tagged # state and LOCALVERSION= is not specified if test "${LOCALVERSION+set}" != "set"; then scm=$(scm_version --short) res="$res${scm:++}" fi fi
(In reply to comment #21) > (In reply to comment #20) > > The '+' isn't generated by the setlocalversion script; it's added by the > > makefile (which ignores the setlocalversion output in this case) when > > CONFIG_LOCALVERSION_AUTO is not set. The Makefile comments specify the exact > > conditions. The section of the Makefile that actually generates the version > > number starts at line 885 in current git. > I'm sorry, but you're wrong. The + is clearly generated by the setlocalversion > script in the latest tree (e467e104bb7 right now). The Makefile _always_ runs > setlocalversion, and there is no other means for the + to be set. The confusion here is quite understandable: the + is generated by the Makefile in -rc3-ish and earlier. The + is generated by the setlocalversion script in -rc4-ish and later.
(In reply to comment #22) > The confusion here is quite understandable: the + is generated by the > Makefile in -rc3-ish and earlier. The + is generated by the > setlocalversion script in -rc4-ish and later. Ah, oops :) I guess I should update my git checkout. But this does underscore the volatility of relying on the internal build scripts used by the kernel. The only user-visible APIish method that I've seen for getting the kernel version is to run 'make kernelversion' or 'make kernelrelease'. Neither of them actually generate any files, so they should be safe to run in sandbox. ('kernelversion' doesn't even require a prepared source directory). If the individual pieces of the version number are required, the versionator eclass scripts could be used to chop it up, I'd think?
'make kernelrelease' DOES require writing if the prepare hasn't been done, and fails in R/O sandbox. kernelrelease: $(if $(wildcard include/config/kernel.release), $(Q)echo $(KERNELRELEASE), \ $(error kernelrelease not valid - run 'make prepare' to update it)) include/config/kernel.release: include/config/auto.conf FORCE $(Q)rm -f $@ $(Q)echo "$(KERNELVERSION)$$($(CONFIG_SHELL) $(srctree)/scripts/setlocalversion $(srctree))" > $@ The other entire thing is avoiding the exec of the Makefile as much as possible. Historically it was because it had side-effects sometimes. We're going to get the '+' output solved, and that's all.
(In reply to comment #24) > We're going to get the '+' output solved, and that's all. And go through this dance all over again next time the build script is changed? It should be pretty clear from all this that the setlocalversion script is not intended to be called by the user, given that these changes occurred so late in the release cycle. When are KV_LOCAL and KV_FULL needed except when building kernel modules? When we're building kernel modules, we should assume a prepared tree and use the actual version string computed by the build system. When we're not building kernel modules, we shouldn't need to determine these things. Using kernel.release (or .kernelrelease for pre-2.6.18) in the build tree also has the advantage of actually working when the kernel was built using 'make LOCALVERSION=...', and would work with all 2.6 kernels in the portage tree.
(In reply to comment #25) > When are KV_LOCAL and KV_FULL needed except when building kernel modules? When > we're building kernel modules, we should assume a prepared tree and use the > actual version string computed by the build system. When we're not building > kernel modules, we shouldn't need to determine these things. I was curious, so I grepped the portage tree, this is what I found: * KV_LOCAL is never used in any ebuild. * KV_FULL is used in two places (other than for building modules): - Descriptive error messages in the udev ebuilds. - Version checking in the oprofile ebuild, using a shell case statement that only checks the first two components of the version number anyways. This should probably be fixed.
Check: - eclasses - eblits libraries in some packages. - overlays?
I want just to summarize what I think should be the behaviour of get_version() in linux-info.eclass. The function is supposed to work also when the kernel is not configured so this means that .config may not exist and that setlocalversion can work only partially (latest kernel versions). The version string returned may be different between a configured and not configured kernel (when using linux-mod.eclass get_version() must return the same string of the kernel build process). The function get_version() fills the KV_MAJOR, KV_MINOR, KV_PATCH, KV_EXTRA, KV_LOCAL and KV_FULL variables. The first four are always present because are in the Makefile, KV_LOCAL can be build in many ways. In principle scripts/setlocalversion provides all the info needed to fill KV_LOCAL but it needs a configured kernel. KV_LOCAL, when is complete, is built as "localversion* files logic" + "LOCALVERSION logic" + "LOCALVERSION_AUTO logic". If the kernel is not configured CONFIG_LOCALVERSION and CONFIG_LOCALVERSION_AUTO are not set. IMHO in this case we should only care about localversion* files. If the kernel is configured I guess we should trust scripts/setlocalversion. If scripts/setlocalversion is not present we should have a fallback. Up to now I put the "localversion* files logic" + "LOCALVERSION logic". I'm too lazy now to check which was the past behaviour. So here is a proposed patch to linux-info.eclass.
Created attachment 239187 [details, diff] linux-info.eclass.patch
(In reply to comment #29) > Created an attachment (id=239187) [details] > linux-info.eclass.patch Doesn't work for me -- instead of '2.6.35-rc4-pomiocik+' gives '2.6.35-rc4+'.
(In reply to comment #30) > (In reply to comment #29) > > Created an attachment (id=239187) [details] [details] > > linux-info.eclass.patch > > Doesn't work for me -- instead of '2.6.35-rc4-pomiocik+' gives '2.6.35-rc4+'. My mistake. After fixing the 'setlocalversion' script to be POSIX-compliant, it gives the correct name.
I came across this bug while wondering why suddenly I all the ebuild I use for external modules (like broadcom-sta, nouveau-drm, etc.) wanted to put their modules into /lib/modules/2.6.35-rc5-Error:/ The patch from Fabio (comment #29) solves the problem for me. We should commit a patch, as more and more users will be unable to compile any external modules.
The patch from #29 don't work for me because it keeps installing the modules in 2.6.35-rc6Error: (using 2.6.35-rc6 vanilla kernel with KBUILD_OUTPUT dir), but i "fixed" it changing the patch line from: KV_LOCAL="${KV_LOCAL}$(cd ${KV_DIR} ; sh ${KV_DIR}/scripts/setlocalversion ${KV_DIR})" to KV_LOCAL="${KV_LOCAL}$(cd ${KV_OUT_DIR} ; sh ${KV_DIR}/scripts/setlocalversion ${KV_DIR})" Not sure if is a proper fix, but it worked in my case.
(In reply to comment #29) > Created an attachment (id=239187) [details] > linux-info.eclass.patch I have now committed a modified version of this patch. 1. the sh call to setlocalversion is replaced with bash, due to the POSIX issue. 2. KV_OUT_DIR is pwd for the running of said script. Thanks for the patch Fabio.
(In reply to comment #34) > (In reply to comment #29) > > Created an attachment (id=239187) [details] [details] > > linux-info.eclass.patch > I have now committed a modified version of this patch. > 1. the sh call to setlocalversion is replaced with bash, due to the POSIX > issue. > 2. KV_OUT_DIR is pwd for the running of said script. Which kernel versions was this tested on? Using the updated linux-info.eclass I find that out-of-tree modules, e.g nvidia-drivers, are installed into the correct modules directory for 2.6.35 (e.g. 2.6.35-20100803-00-00001-gde770fe) but not for 2.6.34.2. In the latter case, where the correct directory is, e.g., 2.6.34.2-20100803-00-00005-gf199ebb, then the external modules get installed into 2.6.34.2-00005-gf199ebb, the problem being that for 2.6.34.2 the value of CONFIG_LOCALVERSION ("-20100803-00") is not included in the output directory name. In both cases the kernel source is a local clone of the git tree on kernel.org rather than a packaged tarball. But I don't think that should impact at all. Hope this helps.
2.6.35-rc2-10374-g3975d16 is my local kernel version that I confirmed it getting the correct directory on. Can you trace why it seems to work for 2.6.35 but not 2.6.34.2? Maybe more upstream makefile changes?
Created attachment 241545 [details, diff] Patch to fix everything. OK, here's a really simple patch against the latest portage tree that uses the actual resources given to us by the kernel build system to compute the version string. We don't change any externally visible behaviour, and we still work on unprepared trees. We exploit the fact that no ebuild can possibly care about KV_LOCAL on unprepared trees, and just set it to the empty string in this case. Otherwise, we set it to the contents of kernel.release with the first bits stripped off. This guarantees that KV_LOCAL is set correctly on all prepared trees corresponding to kernel versions since the 2.6.18 merge window (more than four years ago now). Compatibility with kernels as far back as 2.6.16 is still there (via .kernelrelease), but the git suffix is not added in this case. I don't think this is really worth caring about: kernels built from the 2.6.16 sources in the portage tree will work (other LOCALVERSION magic is remains correct). Earlier kernel versions will have KV_LOCAL set to the empty string. Lastly, this change automatically gives us support for 'make LOCALVERSION=foo' builds, which were previously completely unsupported by the eclass.
(In reply to comment #37) > Created an attachment (id=241545) [details] Nope, this still gets it wrong: /lib/modules/2.6.35-rc42.6.35-rc2-10374-g3975d16/block/vhba.ko (should be 2.6.35-rc2-10374-g3975d16) Specifically, kernel.release can be out of date in many cases, and we cannot trust it.
> Specifically, kernel.release can be out of date in many cases, and we cannot > trust it. Yes, if you do git checkouts to bounce around without doing a prepare, your module builds will fail. News at 11. Please tell me the cases when both of the following are true: a) kernel.release is invalid b) KV_FULL must be correct. The only case I can think of is module installation, which requires a prepared tree. If your tree is not prepared for module installation, *you* get to keep the pieces.
Also keep in mind that the current version in the portage tree has broken _every_ kernel version prior to 2.6.35 (See comment #35). This is *not cool*.
Created attachment 241555 [details, diff] Fix everything, v2 Here's the same patch, except it will detect the scenario reported in comment #38 and set KV_LOCAL to the empty string in that case.
In reply to comment #39) > Yes, if you do git checkouts to bounce around without doing a prepare, your > module builds will fail. News at 11. Git checkouts are the easiest way to produce it, but any case where prepare wasn't run to update the kernel.release can produce it. > Please tell me the cases when both of the following are true: > a) kernel.release is invalid > b) KV_FULL must be correct. Either we need to generate the correct KV_FULL and value for kernel.release ourselves, or we need to be able to detect it being mismatched and die. > The only case I can think of is module installation, which requires a prepared > tree. If your tree is not prepared for module installation, *you* get to keep > the pieces. If you toggle CONFIG_LOCALVERSION_AUTO without running prepare, your code generates the wrong output too. (make kernelrelease also generates the wrong output) The rule for making kernel.release is the following: === include/config/kernel.release: include/config/auto.conf FORCE $(Q)rm -f $@ $(Q)echo "$(KERNELVERSION)$$($(CONFIG_SHELL)$(srctree)/scripts/setlocalversion $(srctree))" > $@ === So we absolutely should be able to run that ourselves and get the intended output, and that's what the latest version attempts to do. As for breaking older kernels, I'm retesting that now.
(In reply to comment #42) > The rule for making kernel.release is the following: > === > include/config/kernel.release: include/config/auto.conf FORCE > $(Q)rm -f $@ > $(Q)echo > "$(KERNELVERSION)$$($(CONFIG_SHELL)$(srctree)/scripts/setlocalversion > $(srctree))" > $@ > === You are forgetting that there are at least *three* different rules for making kernel.release: (a) The rule prior to around 2.6.35-rc2 (b) The rule between around 2.6.35-rc2 and around 2.6.35-rc4 (c) The rule after around 2.6.35-rc4 Plus any others that I missed, plus any future rules that might be written according to the whim of the kbuild maintainer. We could argue about whether (b) is worth supporting, but there should be no question that we _must_ support both (a) and (c). On the other hand, reading kernel.release directly gives us full support for all kernels in the past four years without any special cases, and partial support for kernels earlier than that.
(In reply to comment #42) > In reply to comment #39) > Git checkouts are the easiest way to produce it, but any case where prepare > wasn't run to update the kernel.release can produce it. > > > Please tell me the cases when both of the following are true: > > a) kernel.release is invalid > > b) KV_FULL must be correct. > Either we need to generate the correct KV_FULL and value for kernel.release > ourselves, or we need to be able to detect it being mismatched and die. Please remember that it is _impossible_ to correctly compute the localversion on an unprepared tree. Computing the local version requires information only available at prepare time, namely all of the following: * The CONFIG_LOCALVERSION parameter * The LOCALVERSION variable (if any) passed to make at prepare time (this is currently not supported by the eclass) * The output of the setlocalversion script (which in turn depends on the CONFIG_LOCALVERSION parameter). The new patch detects completely bogus kernel.release values and just drops out KV_LOCAL since there's no way to correctly compute it. Since it's impossible to compute KV_FULL correctly on unprepared trees, there is no point worrying about it being 100% correct on unprepared trees (we can guarantee that the non-local parts are right, which my patch does).
(In reply to comment #44) > * The output of the setlocalversion script (which in turn depends on the > CONFIG_LOCALVERSION parameter). Brain-o, that should be CONFIG_LOCALVERSION_AUTO.
(In reply to comment #44) > Please remember that it is _impossible_ to correctly compute the localversion > on an unprepared tree. Computing the local version requires information only > available at prepare time, namely all of the following: On a completely unprepared tree I agree it's impossible, and that part I do not dispute, however my concern is for the mismatch case. In which case I think the best option is to simply die when the prepare that was previously run is not sufficient. On a completely unprepared tree, we should probably die as well. Those die calls probably belongs best in the check_kernel_built function, as die is prohibited in the linux-info:get_version function. > * The CONFIG_LOCALVERSION parameter > * The LOCALVERSION variable (if any) passed to make at prepare time > (this is currently not supported by the eclass) > * The output of the setlocalversion script (which in turn depends on the > CONFIG_LOCALVERSION parameter). Only in the changes introduced between 2.6.35-rc3 and 2.6.35-rc4 (not rc2 as you suggested before) does setlocalversion look at the CONFIG_LOCALVERSION* variables and the LOCALVERSION env var. Prior to that, setlocalversion was ONLY called if CONFIG_LOCALVERSION_AUTO was true. > The new patch detects completely bogus kernel.release values and just drops out > KV_LOCAL since there's no way to correctly compute it. See my comments about dieing.
(In reply to comment #46) > (In reply to comment #44) > > Please remember that it is _impossible_ to correctly compute the > > localversion on an unprepared tree. Computing the local version requires > > information only available at prepare time, namely all of the following: > On a completely unprepared tree I agree it's impossible, and that part I do > not dispute, however my concern is for the mismatch case. In which case I > think the best option is to simply die when the prepare that was previously > run is not sufficient. On a completely unprepared tree, we should probably > die as well. Those die calls probably belongs best in the check_kernel_built > function, as die is prohibited in the linux-info:get_version function. Feel free to make the module build die if it's obviously going to fail later. That is an issue which is not really related to this bug, which is about the version detection being totally wrong. We already die on unconfigured trees for module builds, isn't that good enough? If the user breaks their tree, then tries to build modules against it, they get to keep both pieces. > > * The output of the setlocalversion script (which in turn depends on the > > CONFIG_LOCALVERSION parameter). > Only in the changes introduced between 2.6.35-rc3 and 2.6.35-rc4 (not rc2 as > you suggested before) does setlocalversion look at the CONFIG_LOCALVERSION* > variables and the LOCALVERSION env var. Prior to that, setlocalversion was ONLY > called if CONFIG_LOCALVERSION_AUTO was true. Sorry if it wasn't clear, what I meant to convey here is that the output of setlocalversion is useless prior to kernel configuration: i.e. one needs to know the CONFIG_LOCALVERSION_AUTO setting, which is not known on an unprepared tree. > > The new patch detects completely bogus kernel.release values and just drops > > out KV_LOCAL since there's no way to correctly compute it. > See my comments about dieing. I agree that we can't die here because, as you have noted, this needs to work on unprepared trees. This is why the patch *does not die*! Setting KV_LOCAL to empty when it's detected to be completely bogus is a natural way to solve this. Only if we are building modules is it suitable to die here. My goal is to make the eclass work *right now*. The patch was designed to be as unintrusive as possible, and deliberately doesn't remove any of the outputs of the linux-info eclass. More intrusive changes (such as removing KV_FULL from linux-info completely, since it's bogus) can be made in a later patch. Right now, kernel module installation on Gentoo is *totally borked*, has been for weeks, and needs to be fixed. Let's have the smallest degree of faith in Gentoo users: if their kernel trees are screwed up, module installation isn't guaranteed to work.
Merged your v2 patch for now, with extra comments. Please submit an additional patch to apply on top of present state, that implements the TODOs. (In reply to comment #47) > Feel free to make the module build die if it's obviously going to fail later. > That is an issue which is not really related to this bug, which is about the > version detection being totally wrong. The problem is detecting those. Your existing tmplocal hack is not enough either. See the TODO blocks I added to the eclass. > We already die on unconfigured trees for module builds, isn't that good enough? > If the user breaks their tree, then tries to build modules against it, they get > to keep both pieces. I agree, but we need to detect it, instead of generating bad packages as presently happens with your patch. > I agree that we can't die here because, as you have noted, this needs to work > on unprepared trees. This is why the patch *does not die*! Setting KV_LOCAL > to empty when it's detected to be completely bogus is a natural way to solve > this. Only if we are building modules is it suitable to die here. TODO is there to export an env var and return 1, then we can use that from the get_version consumers. > More intrusive changes (such as removing KV_FULL > from linux-info completely, since it's bogus) can be made in a later patch. It needs to stay, it's used elsewhere. > Right now, kernel module installation on Gentoo is *totally borked*, has been > for weeks, and needs to be fixed. It's not totally borked. If it was, there would be a lot more bugs open. It's only broken for the LOCALVERSION users, there haven't been any dupe bugs of this one even.
(In reply to comment #48) > Merged your v2 patch for now, with extra comments. Please submit an additional > patch to apply on top of present state, that implements the TODOs. > > (In reply to comment #47) > > Feel free to make the module build die if it's obviously going to fail later. > > That is an issue which is not really related to this bug, which is about the > > version detection being totally wrong. > The problem is detecting those. Your existing tmplocal hack is not enough > either. See the TODO blocks I added to the eclass. > > > We already die on unconfigured trees for module builds, isn't that good > > enough? If the user breaks their tree, then tries to build modules against > > it, they get to keep both pieces. > I agree, but we need to detect it, instead of generating bad packages as > presently happens with your patch. There are countless ways to cause module builds to generate bad packages, and Rice's theorem tells us that it's impossible to write a computer program to determine conclusively whether it'll work or not without actually building and loading the kernel module. What if the user runs 'vi /usr/src/linux/include/config/kernel.release'? Should we try and detect that too? This is the beginning of a slippery slope that continues into ridiculousness. However, bailing out if the tmplocal check fails seems entirely reasonable. Also, this isn't really a new problem: for example, changing the CONFIG_LOCALVERSION_AUTO parameter with 'make menuconfig' and then not running 'make prepare' to update include/config/auto.conf will cause scripts/setlocalversion to return settings based on the old value.
(In reply to comment #49) > There are countless ways to cause module builds to generate bad packages, and > Rice's theorem tells us that it's impossible to write a computer program to > determine conclusively whether it'll work or not without actually building and > loading the kernel module. Yet we can be much more certain if we run the setlocalversion ourselves, instead of relying on the cached result of it. Best would be if the kernel Makefile had a rule that computed and just output the version to stdout, instead of the present choices to only write to kernel.release OR read from it. > What if the user runs 'vi /usr/src/linux/include/config/kernel.release'? > Should we try and detect that too? This is the beginning of a slippery slope > that continues into ridiculousness. See why I say I would like to run setlocalversion ourselves. > Also, this isn't really a new problem: for example, changing the > CONFIG_LOCALVERSION_AUTO parameter with 'make menuconfig' and then not > running 'make prepare' to update include/config/auto.conf will cause > scripts/setlocalversion to return settings based on the old value. Yup, this is related. 1. does setlocalversion exist? 2. does the Makefile contain CONFIG_LOCALVERSION_AUTO? 3. does it contain CONFIG_LOCALVERSION? 4. does it contain CONFIG_LOCALVERSION_AUTO? Based on the above, I believe we can reliably build the better results than the cached kernel.release, for the case of ALL kernels in the last 5 years.
(In reply to comment #50) > Yet we can be much more certain if we run the setlocalversion ourselves, > instead of relying on the cached result of it. Best would be if the kernel > Makefile had a rule that computed and just output the version to stdout, > instead of the present choices to only write to kernel.release OR read from > it. Except not, because the setlocalversion script itself depends on an otherwise prepared tree: it depends on an up-to-date include/config/auto.conf, which is not created until 'make prepare' time (really 'make silentoldconfig'). > > What if the user runs 'vi /usr/src/linux/include/config/kernel.release'? > > Should we try and detect that too? This is the beginning of a slippery slope > > that continues into ridiculousness. > See why I say I would like to run setlocalversion ourselves. I was more trying to make the point that the user has an editor and can edit things to not work. How about 'vi /usr/src/linux/scripts/setlocalversion' instead? > > Also, this isn't really a new problem: for example, changing the > > CONFIG_LOCALVERSION_AUTO parameter with 'make menuconfig' and then not > > running 'make prepare' to update include/config/auto.conf will cause > > scripts/setlocalversion to return settings based on the old value. > Yup, this is related. > > 1. does setlocalversion exist? > 2. does the Makefile contain CONFIG_LOCALVERSION_AUTO? > 3. does it contain CONFIG_LOCALVERSION? > 4. does it contain CONFIG_LOCALVERSION_AUTO? This isn't sufficient, because as mentioned above, the setlocalversion script depends on include/config/auto.conf, which is out of date as often as kernel.release is. We also miss LOCALVERSION passed on the command line at build time. > Based on the above, I believe we can reliably build the better results than > the cached kernel.release, for the case of ALL kernels in the last 5 years. But you still haven't solved the "problem" where the user updates the config without running make prepare. For example, include/generated/autoconf.h and include/generated/utsrelease.h, which affect the actual module compilation process, are not updated in this case. So even if you do correctly determine the module version for the kernel as configured, you end up installing it to the wrong place anyway because it was just built (perhaps successfully) against the *old* configuration! This case of "half-prepared" trees is completely the user's fault, and is not worth trying to hack around in my opinion.
(In reply to comment #51) > This case of "half-prepared" trees is completely the user's fault, and is not > worth trying to hack around in my opinion. Ok, how about this for the moment: kernel.release/.kernelrelease must exist, and be NEWER than the .config and any localversion files. If not, we bail. This won't detect a minor git update however, but it's probably the closest we can come. If the user has edited it manually, they either know exactly what they are doing, or are way out of their depth.
*** Bug 331467 has been marked as a duplicate of this bug. ***
*** Bug 328243 has been marked as a duplicate of this bug. ***
Has this patch been tested with a kernel 2.4 also? When I created a very similar patch for my duplicate bug # 331467, I was concerned about whether this would work for 2.4 kernels also. And 2.4.37 is still in Portage.
