Re: [pve-devel] [PATCH proxmox-firewall 33/37] firewall: add files for debian packaging

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

just looked at the packaging, mostly related to clean building, but not
only.

On April 2, 2024 7:16 pm, Stefan Hanreich wrote:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  Makefile                        | 93 +++++++++++++++++++++++++++++++++
>  debian/changelog                |  5 ++
>  debian/control                  | 31 +++++++++++
>  debian/copyright                | 16 ++++++
>  debian/proxmox-firewall.service | 16 ++++++
>  debian/proxmox-firewall.timer   | 11 ++++
>  debian/rules                    | 14 +++++
>  debian/source/format            |  1 +
>  defines.mk                      | 13 +++++
>  9 files changed, 200 insertions(+)
>  create mode 100644 Makefile
>  create mode 100644 debian/changelog
>  create mode 100644 debian/control
>  create mode 100644 debian/copyright
>  create mode 100644 debian/proxmox-firewall.service
>  create mode 100644 debian/proxmox-firewall.timer
>  create mode 100644 debian/rules
>  create mode 100644 debian/source/format
>  create mode 100644 defines.mk
> 
> diff --git a/Makefile b/Makefile
> new file mode 100644
> index 0000000..984c318
> --- /dev/null
> +++ b/Makefile
> @@ -0,0 +1,93 @@
> +include /usr/share/dpkg/pkg-info.mk
> +include /usr/share/dpkg/architecture.mk
> +include defines.mk
> +
> +PACKAGE=proxmox-firewall
> +BUILDDIR ?= $(PACKAGE)-$(DEB_VERSION_UPSTREAM)
> +
> +
> +DEB=$(PACKAGE)_$(DEB_VERSION_UPSTREAM_REVISION)_$(DEB_HOST_ARCH).deb
> +DBG_DEB=$(PACKAGE)-dbgsym_$(DEB_VERSION_UPSTREAM_REVISION)_$(DEB_HOST_ARCH).deb
> +DSC=rust-$(PACKAGE)_$(DEB_VERSION_UPSTREAM_REVISION).dsc

this doesn't match d/control ;)

> +
> +DEBS = $(DEB) $(DBG_DEB)
> +
> +ifeq ($(BUILD_MODE), release)

you need to set/export this in d/rules, else the package will contain a
debug build..

> +CARGO_BUILD_ARGS += --release
> +COMPILEDIR := target/release
> +else
> +COMPILEDIR := target/debug
> +endif
> +
> +USR_BIN := \
> +	proxmox-firewall
> +
> +COMPILED_BINS := \
> +	$(addprefix $(COMPILEDIR)/,$(USR_BIN))
> +
> +all: cargo-build
> +
> +.PHONY: cargo-build
> +cargo-build:
> +	cargo build $(CARGO_BUILD_ARGS)
> +
> +$(COMPILED_BINS): cargo-build
> +
> +install: $(COMPILED_BINS)
> +	install -dm755 $(DESTDIR)$(SBINDIR)
> +	$(foreach i,$(USR_BIN), \
> +	    install -m755 $(COMPILEDIR)/$(i) $(DESTDIR)$(SBINDIR)/ ;)

I am not sure this (and all the helper stuff above that only exists for
it) make much sense. maybe we could simply get `$(CARGO) install` to
work here, and then let the packging put it into /usr/sbin ?

> +
> +update-dcontrol: #$(BUILDDIR)
> +	debcargo package \
> +	  --config debian/debcargo.toml \
> +	  --changelog-ready \
> +	  --no-overlay-write-back \
> +	  --directory $(BUILDDIR) \
> +	  $(PACKAGE) \
> +	  $(shell dpkg-parsechangelog -l debian/changelog -SVersion | sed -e 's/-.*//')
> +	cat $(BUILDDIR)/debian/control debian/control.extra > debian/control
> +	wrap-and-sort -t -k -f debian/control

this doesn't work because there is no debian/debcargo.toml and also no
debian/control.extra ;) since debcargo doesn't work with workspaces
(yet), it probably doesn't make sense to leave this in.. one just has to
remember to update d/control when updating any of the Cargo.toml files
w.r.t. dependencies or features.

> +
> +.PHONY: build
> +build: $(BUILDDIR)
> +$(BUILDDIR):
> +	rm -rf $@ $@.tmp; mkdir $@.tmp
> +	cp -a proxmox-firewall proxmox-nftables proxmox-ve-config debian Cargo.toml Makefile defines.mk $@.tmp/

this is missing the .cargo dir, which doesn't matter (much) for building
directly via `make deb` (since cargo will look in parent dirs), but
completely breaks for `make dsc` or `make sbuild`. but I suggest
handling that in d/rules (see comment there)

> +	mv $@.tmp $@
> +
> +.PHONY: deb
> +deb: $(DEB)
> +$(HELPER_DEB) $(DBG_DEB) $(HELPER_DBG_DEB) $(DOC_DEB): $(DEB)
> +$(DEB): $(BUILDDIR)
> +	cd $(BUILDDIR); dpkg-buildpackage -b -us -uc --no-pre-clean
> +	lintian $(DEB) $(DOC_DEB) $(HELPER_DEB)
> +
> +.PHONY: test
> +test:
> +	cargo test
> +
> +.PHONY: dsc
> +dsc:
> +	rm -rf $(BUILDDIR) $(DSC)
> +	$(MAKE) $(DSC)
> +	lintian $(DSC)
> +$(DSC): $(BUILDDIR)
> +	cd $(BUILDDIR); dpkg-buildpackage -S -us -uc -d -nc
> +
> +sbuild: $(DSC)
> +	sbuild $<
> +
> +.PHONY: dinstall
> +dinstall: $(DEB)
> +	dpkg -i $(DEB) $(DBG_DEB) $(DOC_DEB)
> +
> +.PHONY: distclean
> +distclean: clean
> +
> +.PHONY: clean
> +clean:
> +	cargo clean
> +	rm -f *.deb *.build *.buildinfo *.changes *.dsc rust-$(PACKAGE)*.tar*
> +	rm -rf $(PACKAGE)-[0-9]*/
> +	find . -name '*~' -exec rm {} ';'
> diff --git a/debian/changelog b/debian/changelog
> new file mode 100644
> index 0000000..7918ec9
> --- /dev/null
> +++ b/debian/changelog
> @@ -0,0 +1,5 @@
> +proxmox-firewall (0.1-1) UNRELEASED; urgency=medium

the `-1` and `debian/source/format` disagree (again, only a problem when
attempting building via sbuild)

> +
> +  * Initial release.
> +
> + -- Stefan Hanreich <s.hanreich@proxmox.com>  Thu, 07 Mar 2024 10:15:10 +0100
> diff --git a/debian/control b/debian/control
> new file mode 100644
> index 0000000..e04ce68
> --- /dev/null
> +++ b/debian/control
> @@ -0,0 +1,31 @@
> +Source: proxmox-firewall
> +Section: admin
> +Priority: optional
> +Maintainer: Proxmox Support Team <support@proxmox.com>
> +Build-Depends: cargo:native,
> +	       debhelper-compat (= 13),
> +	       dh-cargo (>= 25),

you aren't actually using that though? (and probably shouldn't, it's
mainly for "simple" crate <-> package situations)

> +	       librust-anyhow-1+default-dev,
> +	       librust-env-logger-0.10+default-dev,
> +	       librust-log-0.4+default-dev (>= 0.4.17-~~),
> +	       librust-nix-0.26+default-dev (>= 0.26.1-~~),
> +	       librust-serde-1+default-dev,
> +	       librust-serde-1+derive-dev,
> +	       librust-serde-json-1+default-dev,
> +	       librust-serde-plain-1+default-dev,
> +	       librust-serde-plain-1+default-dev,
> +	       librust-serde-with+default-dev,
> +	       librust-libc-0.2+default-dev,
> +	       librust-proxmox-schema-3+default-dev,

this is missing at least libnftables-dev and netbase

> +Standards-Version: 4.6.2
> +Homepage: https://www.proxmox.com
> +
> +Package: proxmox-firewall
> +Architecture: any
> +Conflicts: ulogd,
> +Depends: ${misc:Depends}, ${shlibs:Depends},
> +	 pve-firewall,
> +	 nftables,

this is missing at least netbase ;)

> +Description: Proxmox VE nft Firewall
> + This package contains a nftables-based implementation of the Proxmox VE
> + Firewall
> diff --git a/debian/copyright b/debian/copyright
> new file mode 100644
> index 0000000..fe09a1b
> --- /dev/null
> +++ b/debian/copyright
> @@ -0,0 +1,16 @@
> +Copyright (C) 2018-2024 Proxmox Server Solutions GmbH
> +
> +This software is written by Proxmox Server Solutions GmbH <support@proxmox.com>
> +
> +This program is free software: you can redistribute it and/or modify
> +it under the terms of the GNU Affero 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 Affero General Public License for more details.
> +
> +You should have received a copy of the GNU Affero General Public License
> +along with this program.  If not, see <http://www.gnu.org/licenses/>.
> diff --git a/debian/proxmox-firewall.service b/debian/proxmox-firewall.service
> new file mode 100644
> index 0000000..5f9bf4b
> --- /dev/null
> +++ b/debian/proxmox-firewall.service
> @@ -0,0 +1,16 @@
> +[Unit]
> +Description=Proxmox VE nftables firewall
> +ConditionPathExists=/usr/sbin/proxmox-firewall

when would this path not exist?

> +Wants=pve-cluster.service pvefw-logger.service
> +After=pvefw-logger.service pve-cluster.service network.target systemd-modules-load.service

should the timer also get this to avoid premature starting? or am I
misremembering how timers and their services interact :)

> +DefaultDependencies=no
> +Before=shutdown.target
> +Conflicts=shutdown.target
> +
> +[Service]
> +ExecStart=/usr/sbin/proxmox-firewall
> +Type=oneshot
> +
> +[Install]
> +WantedBy=multi-user.target
> +

so this is started by the timer below (on boot and then every 5s) but
also enabled and thus started once on boot? that seems confusing ;)

> diff --git a/debian/proxmox-firewall.timer b/debian/proxmox-firewall.timer
> new file mode 100644
> index 0000000..d051102
> --- /dev/null
> +++ b/debian/proxmox-firewall.timer
> @@ -0,0 +1,11 @@
> +[Unit]
> +Description=Proxmox VE nft Firewall timer

capitalisation compared to the service above (and also, nft vs nftables)

> +
> +[Timer]
> +OnBootSec=1s
> +OnUnitInactiveSec=5s
> +Unit=proxmox-firewall.service
> +
> +[Install]
> +WantedBy=timers.target
> +
> diff --git a/debian/rules b/debian/rules
> new file mode 100644

d/rules should be executable :)

> index 0000000..5539a00
> --- /dev/null
> +++ b/debian/rules
> @@ -0,0 +1,14 @@
> +#!/usr/bin/make -f
> +
> +# Uncomment this to turn on verbose mode.
> +#export DH_VERBOSE=1
> +
> +%:
> +	dh $@
> +

I would suggest calling the cargo wrapper here to create a proper
.cargo/config file for a clean build. you can look at proxmox-backup for
inspiration ;)

here's a starting point:

diff --git a/debian/rules b/debian/rules
index 5539a00..bbb4d0a 100644
--- a/debian/rules
+++ b/debian/rules
@@ -1,14 +1,31 @@
 #!/usr/bin/make -f
 
 # Uncomment this to turn on verbose mode.
-#export DH_VERBOSE=1
+export DH_VERBOSE=1
+
+include /usr/share/dpkg/pkg-info.mk
+include /usr/share/rustc/architecture.mk
+
+export BUILD_MODE=release
+
+CARGO=/usr/share/cargo/bin/cargo
+
+export CFLAGS CXXFLAGS CPPFLAGS LDFLAGS
+export DEB_HOST_RUST_TYPE DEB_HOST_GNU_TYPE
+export CARGO_HOME = $(CURDIR)/debian/cargo_home
+
+export DEB_CARGO_CRATE=proxmox-firewall_$(DEB_VERSION_UPSTREAM)
+export DEB_CARGO_PACKAGE=proxmox-firewall
 
 %:
 	dh $@
 
+override_dh_auto_configure:
+	@perl -ne 'if (/^version\s*=\s*"(\d+(?:\.\d+)+)"/) { my $$v_cargo = $$1; my $$v_deb = "$(DEB_VERSION_UPSTREAM)"; \
+	    die "ERROR: d/changelog <-> Cargo.toml version mismatch: $$v_cargo != $$v_deb\n" if $$v_cargo ne $$v_deb; exit(0); }' Cargo.toml
+	$(CARGO) prepare-debian $(CURDIR)/debian/cargo_registry --link-from-system
+	dh_auto_configure
+
 override_dh_installsystemd:
 	dh_installsystemd --no-start proxmox-firewall.service
 	dh_installsystemd proxmox-firewall.timer


we probably also want `cargo` in the Makefile to be $(CARGO)
(defaulting to `cargo` there if not set). that way, `cargo build` and
`cargo test` also go through the wrapper and pick up any special sauce

> +override_dh_installsystemd:
> +	dh_installsystemd --no-start proxmox-firewall.service
> +	dh_installsystemd proxmox-firewall.timer
> +
> +override_dh_installinit:
> +

don't think this dh_installinit override is needed?

> diff --git a/debian/source/format b/debian/source/format
> new file mode 100644
> index 0000000..89ae9db
> --- /dev/null
> +++ b/debian/source/format
> @@ -0,0 +1 @@
> +3.0 (native)

native means no Debian revision in the version, see above w.r.t.
d/changelog

> diff --git a/defines.mk b/defines.mk
> new file mode 100644
> index 0000000..e01164d
> --- /dev/null
> +++ b/defines.mk
> @@ -0,0 +1,13 @@
> +PREFIX = /usr
> +BINDIR = $(PREFIX)/bin
> +SBINDIR = $(PREFIX)/sbin
> +LIBDIR = $(PREFIX)/lib
> +LIBEXECDIR = $(LIBDIR)
> +DATAROOTDIR = $(PREFIX)/share
> +MAN1DIR = $(PREFIX)/share/man/man1
> +MAN5DIR = $(PREFIX)/share/man/man5
> +SYSCONFDIR = /etc
> +
> +# For local overrides
> +-include local.mak
> +
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>