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@xxxxxxxxxx> 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
_______________________________________________
Anaconda-devel mailing list -- anaconda-devel@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to anaconda-devel-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/anaconda-devel@xxxxxxxxxxxxxxxxxxxxxxx
Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
