From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] addon rsnapshot
Date: Wed, 22 Mar 2023 10:01:10 +0000 [thread overview]
Message-ID: <44451D9E-0845-4403-A32C-03FD38664E87@ipfire.org> (raw)
In-Reply-To: <20230319125027.1488265-1-gerd@hoerst.net>
[-- Attachment #1: Type: text/plain, Size: 12942 bytes --]
Hello Gerd,
Thank you for your submission.
I would once again like to highlight that we have collected a large number of DOs and DONTs here:
https://wiki.ipfire.org/devel/submit-patches
That process is needed because there are technical requirements for most steps, and secondly it explains that to work together with other developers.
In this instance, this is a patch that does not at all say what this is supposed to do, what the benefits are, why we should ship this at all, etc. There is no commit message and not even the subject includes any purpose. Just the name of the package.
It is very important to justify *any* patch as explained here:
https://wiki.ipfire.org/devel/git/commit-messages
We need to know the “why”. Arguably this could be rather short when the patch fixes a typo, but adding a new package which will create maintenance work for the whole team should be justified.
> On 19 Mar 2023, at 12:50, Gerd Hoerst <gerd(a)hoerst.net> wrote:
>
> Signed-off-by: Gerd Hoerst <gerd(a)hoerst.net>
> ---
> config/rootfiles/packages/rsnapshot | 6 ++
> lfs/rsnapshot | 86 +++++++++++++++++++++++++++++
> make.sh | 1 +
> src/paks/rsnapshot/install.sh | 27 +++++++++
> src/paks/rsnapshot/uninstall.sh | 26 +++++++++
> src/paks/rsnapshot/update.sh | 26 +++++++++
> 6 files changed, 172 insertions(+)
> create mode 100644 config/rootfiles/packages/rsnapshot
> create mode 100644 lfs/rsnapshot
> create mode 100644 src/paks/rsnapshot/install.sh
> create mode 100644 src/paks/rsnapshot/uninstall.sh
> create mode 100644 src/paks/rsnapshot/update.sh
>
> diff --git a/config/rootfiles/packages/rsnapshot b/config/rootfiles/packages/rsnapshot
> new file mode 100644
> index 000000000..023498e2c
> --- /dev/null
> +++ b/config/rootfiles/packages/rsnapshot
> @@ -0,0 +1,6 @@
> +usr/bin/rsnapshot
> +usr/bin/rsnapshot-diff
> +etc/rsnapshot.conf
> +#etc/rsnapshot.conf.default
> +#usr/share/man/man1/rsnapshot-diff.1
> +#usr/share/man/man1/rsnapshot.1
The rootfile is not in alphabetical order. That suggests that this was not generated from a clean build. Could you please verify that it includes all files that have been created?
> diff --git a/lfs/rsnapshot b/lfs/rsnapshot
> new file mode 100644
> index 000000000..0445471be
> --- /dev/null
> +++ b/lfs/rsnapshot
> @@ -0,0 +1,86 @@
> +###############################################################################
> +# #
> +# IPFire.org - A linux based firewall #
> +# Copyright (C) 2007-2020 IPFire Team <info(a)ipfire.org> #
> +# #
> +# This program is free software: you can redistribute it and/or modify #
> +# it under the terms of the GNU General Public License as published by #
> +# the Free Software Foundation, either version 3 of the License, or #
> +# (at your option) any later version. #
> +# #
> +# This program is distributed in the hope that it will be useful, #
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of #
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the #
> +# GNU General Public License for more details. #
> +# #
> +# You should have received a copy of the GNU General Public License #
> +# along with this program. If not, see <http://www.gnu.org/licenses/>. #
> +# #
> +###############################################################################
> +
> +###############################################################################
> +# Definitions
> +###############################################################################
> +
> +include Config
> +
> +VER = 1.4.5
> +
> +THISAPP = rsnapshot-$(VER)
> +DL_FILE = $(THISAPP).tar.gz
> +DL_FROM = https://github.com/rsnapshot/rsnapshot/releases/download/1.4.5
> +DIR_APP = $(DIR_SRC)/$(THISAPP)
> +TARGET = $(DIR_INFO)/$(THISAPP)
> +PROG = rsnapshot
> +PAK_VER = 1
> +DEPS = "rsync"
DEPS should not be quoted.
> +###############################################################################
> +# Top-level Rules
> +###############################################################################
> +
> +objects = $(DL_FILE)
> +
> +$(DL_FILE) = $(DL_FROM)/$(DL_FILE)
> +
> +$(DL_FILE)_BLAKE2 = 2a668aa16991b2b4e611c6204cdcd0e8c9593e5f0af5ea89e787a578e73b6f5987514cd7d0252bb78aea1b157ef85aea947686111ca9e3befdb2a8cef0aa9ecd
> +
> +install : $(TARGET)
> +
> +check : $(patsubst %,$(DIR_CHK)/%,$(objects))
> +
> +download :$(patsubst %,$(DIR_DL)/%,$(objects))
> +b2 : $(subst %,%_BLAKE2,$(objects))
> +
> +dist:
> + @$(PAK)
> +###############################################################################
> +# Downloading, checking, md5sum
> +###############################################################################
> +
> +$(patsubst %,$(DIR_CHK)/%,$(objects)) :
> + @$(CHECK)
> +
> +$(patsubst %,$(DIR_DL)/%,$(objects)) :
> + @$(LOAD)
> +
> +$(subst %,%_MD5,$(objects)) :
> + @$(MD5)
> +
> +###############################################################################
> +# Installation Details
> +###############################################################################
> +
> +$(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
> + @$(PREBUILD)
> + @rm -rf $(DIR_APP) && cd $(DIR_SRC) && tar zxf $(DIR_DL)/$(DL_FILE)
> + cd $(DIR_APP) && \
> + ./configure \
> + --prefix=/usr \
> + --sysconfdir=/etc \
> + --bindir=/usr/bin \
> +
> + cd $(DIR_APP) && make $(MAKETUNING)
> + cd $(DIR_APP) && make install
> + cp -avfr $(DIR_CONF)/$(PROG)/* /
> + @rm -rf $(DIR_APP)
> + @$(POSTBUILD)
> diff --git a/make.sh b/make.sh
> index 3b7f9850c..59d2971da 100755
> --- a/make.sh
> +++ b/make.sh
> @@ -1478,6 +1478,7 @@ buildipfire() {
> lfsmake2 libmpeg2
> lfsmake2 gnump3d
> lfsmake2 rsync
> + lfsmake2 rsnapshot
> lfsmake2 rpcbind
> lfsmake2 keyutils
> lfsmake2 nfs
> diff --git a/src/paks/rsnapshot/install.sh b/src/paks/rsnapshot/install.sh
> new file mode 100644
> index 000000000..9aafb0d56
> --- /dev/null
> +++ b/src/paks/rsnapshot/install.sh
> @@ -0,0 +1,27 @@
> +#!/bin/bash
> +############################################################################
> +# #
> +# This file is part of the IPFire Firewall. #
> +# #
> +# IPFire is free software; you can redistribute it and/or modify #
> +# it under the terms of the GNU General Public License as published by #
> +# the Free Software Foundation; either version 2 of the License, or #
> +# (at your option) any later version. #
> +# #
> +# IPFire is distributed in the hope that it will be useful, #
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of #
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the #
> +# GNU General Public License for more details. #
> +# #
> +# You should have received a copy of the GNU General Public License #
> +# along with IPFire; if not, write to the Free Software #
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA #
> +# #
> +# Copyright (C) 2007 IPFire-Team <info(a)ipfire.org>. #
> +# #
> +############################################################################
> +#
> +. /opt/pakfire/lib/functions.sh
> +
> +extract_files
> +restore_backup ${NAME}
> diff --git a/src/paks/rsnapshot/uninstall.sh b/src/paks/rsnapshot/uninstall.sh
> new file mode 100644
> index 000000000..66f4344eb
> --- /dev/null
> +++ b/src/paks/rsnapshot/uninstall.sh
> @@ -0,0 +1,26 @@
> +#!/bin/bash
> +############################################################################
> +# #
> +# This file is part of the IPFire Firewall. #
> +# #
> +# IPFire is free software; you can redistribute it and/or modify #
> +# it under the terms of the GNU General Public License as published by #
> +# the Free Software Foundation; either version 2 of the License, or #
> +# (at your option) any later version. #
> +# #
> +# IPFire is distributed in the hope that it will be useful, #
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of #
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the #
> +# GNU General Public License for more details. #
> +# #
> +# You should have received a copy of the GNU General Public License #
> +# along with IPFire; if not, write to the Free Software #
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA #
> +# #
> +# Copyright (C) 2007 IPFire-Team <info(a)ipfire.org>. #
> +# #
> +############################################################################
> +#
> +. /opt/pakfire/lib/functions.sh
> +make_backup ${NAME}
> +remove_files
> diff --git a/src/paks/rsnapshot/update.sh b/src/paks/rsnapshot/update.sh
> new file mode 100644
> index 000000000..89c40d0d7
> --- /dev/null
> +++ b/src/paks/rsnapshot/update.sh
> @@ -0,0 +1,26 @@
> +#!/bin/bash
> +############################################################################
> +# #
> +# This file is part of the IPFire Firewall. #
> +# #
> +# IPFire is free software; you can redistribute it and/or modify #
> +# it under the terms of the GNU General Public License as published by #
> +# the Free Software Foundation; either version 2 of the License, or #
> +# (at your option) any later version. #
> +# #
> +# IPFire is distributed in the hope that it will be useful, #
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of #
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the #
> +# GNU General Public License for more details. #
> +# #
> +# You should have received a copy of the GNU General Public License #
> +# along with IPFire; if not, write to the Free Software #
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA #
> +# #
> +# Copyright (C) 2007 IPFire-Team <info(a)ipfire.org>. #
> +# #
> +############################################################################
> +#
> +. /opt/pakfire/lib/functions.sh
> +./uninstall.sh
> +./install.sh
There is no need to have any custom install/uninstall/update scripts when they are not doing anything the default set isn’t doing.
You are calling backup restore functions but there is no backup include file for the configuration file. So these can be removed even if you were to add a backup include file.
Best,
-Michael
> --
> 2.25.1
>
next prev parent reply other threads:[~2023-03-22 10:01 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-19 12:50 Gerd Hoerst
2023-03-19 12:57 ` Gerd Hoerst
2023-03-22 10:01 ` Michael Tremer [this message]
2023-03-22 13:37 ` Gerd Hoerst
2023-03-22 14:09 ` Michael Tremer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=44451D9E-0845-4403-A32C-03FD38664E87@ipfire.org \
--to=michael.tremer@ipfire.org \
--cc=development@lists.ipfire.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox