Hi Philipp,
On Tue, Sep 19, 2023 at 08:14:35PM +0200, Philipp Rudo wrote:
Hi Coiby,
wow, the spec files now look a lot better. There are a few nits but
mostly just typos (or me not understanding things). Thanks for taking
care of this!
Actually I should thank you for carefully reviewing the patch!
On Tue, 19 Sep 2023 12:15:49 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
> Related:
https://bugzilla.redhat.com/show_bug.cgi?id=2121912
>
> This patch splits current kexec-tools into three packages kexec-tools,
> kdump-utils and makedumpfile.
[...]
> When an old version of kexec-tools gets replaced by kdump-utils,
> "%system_post" will be executed in the post scriptlet and the
^^^^^^^^^^^^ ^^^
s/%system_post/%systemd_post/
s/and the purpose is/which has the purpose/ ?
Applied, thanks!
> +Requires: kexec-tools >= 2.0.27-1
The kexec-tools version in this patch is 2.0.27-2. I guess that's what
should be used here instead.
> +# fedora-review needs to install the package to do the check
> +Requires: (makedumpfile or kexec-tools == 2.0.27-1)
Same like above.
In addition, I believe it's just me not understanding how fedora-review
works but haven't you already required kexec-tools 2.0.27-2 above? Or
should it really be -1 here, i.e. the last not split version? But in
that case the content of kdump-utils and kexec-tools are in conflict.
I'm a little bit confused how this line needs to be read...
fedora-review will install kdump-utils to do some checks. But
unfortunately it can't do so because kdump-utils requires makedumpfile
and new kexec-tools so I allow it to require kexec-tools-2.0.27-1 as a
second choice. But in this version, I forgot I've applied your cleanup
patch set so indeed there are conflicts now as pointed out by you.
Fortunately the exception request
https://pagure.io/packaging-committee/issue/1303
is likely to be approved. So there is no need to consider fedora-review
and the next version I can will drop the dependency on kexec-tools-2.0.27-1.
This also avoids the problem that the version comparison "kexec-tools ==
2.0.27-1" somehow doesn't work.
> +#######################################
> +# These are sources for mkdumpramfs
> +# Which is currently in development
> +#######################################
This comment seems to be out-dated. I believe we can just drop it.
Applied, thanks!
> +Source100: dracut-kdump.sh
> +Source101: dracut-module-setup.sh
> +Source102: dracut-monitor_dd_progress
> +Source104: dracut-kdump-emergency.service
> +Source106: dracut-kdump-capture.service
> +Source107: dracut-kdump-emergency.target
> +Source108: dracut-early-kdump.sh
> +Source109: dracut-early-kdump-module-setup.sh
> +
> +Source200: dracut-fadump-init-fadump.sh
> +Source201: dracut-fadump-module-setup.sh
> +
> +%description
> +kdump-utils is reponsible for collecting the crash kernel dump. It builds and
> +loads the kdump initramfs so when a kernel crashes, the system will boot the
> +kdump kernel and initramfs to save the colletecd crash kernel dump to specified
> +target.
> +
> +%build
> +# setup the license and docs
> +# don't copy the files if they exist otherwise building the packages locally
> +# using tools like fedpkg will fail
> +if [ ! -e COPYING ]]; then
Not sure if [...] or [[...]] is the preferred style for spec files. But
[...]] definitely looks odd (and failed for me) ;-)
You are right "[]]" (I meant "[]") is definitely wrong. Thanks for
catching this problem! In practice "[[]]" should also work because
Fedora's /bin/sh is actual bash. And
https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/
also says,
In Fedora, all scriptlets can safely assume they are running under
the bash shell unless a different language has been specified.
despite all the
examples in the above doc use "[]".
> +%install
> +mkdir -p -m755 $RPM_BUILD_ROOT/usr/sbin
> +mkdir -p -m755 $RPM_BUILD_ROOT%{_sysconfdir}/sysconfig
> +mkdir -p -m755 $RPM_BUILD_ROOT%{_sysconfdir}/kdump
> [..]
> +install -m 755 -D %{SOURCE33}
$RPM_BUILD_ROOT%{_prefix}/lib/kernel/install.d/92-crashkernel.install
After seeing how clean the other two spec files look I'm really
considering if we should add a Makefile. Just so we can use
%make_install...
Yeah, there is potential to further simplify the kdump-utils. I'll bring
it to discussion later.
Anyway I've noticed that there are two occurrences with a hard coded
/usr/sbin which probably should be converted to %{?sbindir} (same for
%files).
In addition you could get rid off some calls to mkdir by adding -D to
the install call. And you could get rid off a few calls to install by
grouping them together using the -t option. But that's pure nit picking
from my side similar to cleaning up the Sources on v1.
Applied, thanks for providing a simpler solution!
> +# don't try to systemctl preset the kdump service for old
kexec-tools
> +#
> +#when the old kexec-tools gets removed, this scriptlet will be triggerd to
> +# create a file. So later the posttrans script will know there is no need to
> +# systemctl preset the kdump service.
> +# This solution can be dropped when no users use old version of kexec-tools.
I would prefer if you could add a specific version to the comment.
Otherwise we will forget when no users of the old kexec-tools are
left. Personally I would use this, assuming that the split will get
into F39:
This workaround can be dropped once F40 is released and there is no
more supported update path from the old version of kexec-tools.
I'm not sure when it can be dropped. But dropping it in F40 seems to be
a good choice since RHEL10 will also branch off F40.
> +%define kexec_tools_no_preset %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset
> +%triggerin -- kexec-tools < 2.0.27-2
> +touch %{kexec_tools_no_preset}
> [..]
> +
> +%posttrans
> +# don't try to systemctl preset the kdump service for old kexec-tools
> +if [[ -f %{kexec_tools_no_preset} ]]; then
> + # this if branch can be removed when no users use the old kexec-tools
Same like above. I would prefer if you could add a specific version.
Applied your suggestion to next version, thanks! Btw, I realize
%triggerin actually doesn't work because old versions of kexec-tools
will never be installed for the case we try to take care. So I used
%triggerun in next version.
> [..]
> +%{_bindir}/*
> +%{_datadir}/kdump
Some more nit picking. Why do we provide an empty %{_datadir}/kdump?
Can't we just drop it? It's not a problem with your patch. Just
something I've noticed during review...
Nice catch! Dropped in next version.
> %post
> [..]
> +echo "kexec-tools has been splitted into three packages (kump-utils,
kexec-tools and makedumpfile). Please install kdump-utils if you need the kdump
feature."
s/has been splitted/was split/
s/kump-utils/kdump-utils/
Applied, thanks!
> diff --git a/makedumpfile.spec b/makedumpfile.spec
> new file mode 100644
> index 00000000..20e7edfd
> --- /dev/null
> +++ b/makedumpfile.spec
> @@ -0,0 +1,63 @@
> +%global eppic_ver e8844d3793471163ae4a56d8f95897be9e5bd554
> +%global eppic_shortver %(c=%{eppic_ver}; echo ${c:0:7})
> +Name: makedumpfile
> +Version: 1.7.3
> +Summary: makedumpfile package
> +%description
[..]
> +make a small dumpfile of kdump
The description is pretty short. How about
makedumpfile is a tool to compress and filter out unneeded data from
kernel dumps to reduce its file size. It is typically used with the
kdump mechanism.
> +
> +%prep
> +
Unnecessary new line. At least everywhere else you don't have a new
line when starting a new scriptlet.
Both applied, thanks!
Thanks!
Philipp
--
Best regards,
Coiby