config: make the install/config directory a compile-time option (PIHOLE_INSTALL_DIR) - #2955
config: make the install/config directory a compile-time option (PIHOLE_INSTALL_DIR)#2955cyberb wants to merge 1 commit into
Conversation
|
I think this should also override the |
7461f64 to
10de901
Compare
|
Good call — done. I pushed a change that also routes the So now every |
DL6ER
left a comment
DL6ER
left a comment
There was a problem hiding this comment.
The dual default works well: the CMake cache variable plus the #ifndef fallback in install_paths.h means every TU resolves the same value even when compiled without the -D, keeping files.pid fixed per GHSA-6w8x-p785-6pm4 is correct, and the default build stays byte-for-byte unchanged. One issue needs resolving before we merge, plus a few smaller things.
Teleporter export and import become asymmetric for relocated installs
In src/zip/teleporter.c the archive stores each file under its absolute path minus the leading slash, but the import side matches hardcoded entry names:
const char *extract_files[] = {
"etc/pihole/pihole.toml", // fixed
"etc/pihole/dhcp.leases", // fixed
config.files.gravity.v.s... // derived, matches the export side
};This PR makes the export names depend on PIHOLE_INSTALL_DIR: dhcp.leases via the changed file_path = DHCPLEASESFILE, and pihole.toml transitively through GLOBALTOMLPATH. So with -DPIHOLE_INSTALL_DIR=/opt/pihole/etc the export writes opt/pihole/etc/pihole.toml and opt/pihole/etc/dhcp.leases, while import still only accepts etc/pihole/.... The result is that config and DHCP-lease import silently no-op on a relocated build (they fall into the "Skipping file ..." branch), both on a same-host round-trip and when moving an archive between differently-configured installs.
We should settle the intended contract and make both sides match. If Teleporter archives are meant to stay portable across installs, i.e., the way backups get moved between machines, the canonical etc/pihole/... names should stay fixed on both sides regardless of PIHOLE_INSTALL_DIR, which argues for reverting the dhcp.leases change rather than extending it. If instead archives are treated as install-local, the import extract_files[] entries need to be derived from PIHOLE_INSTALL_DIR too. The fixed-name option is the more robust of the two, as it keeps existing archives importable on any build. Either way the current half-state is the one thing we cannot keep. The gravity and FTL-db entries already derive from the runtime config value symmetrically, so only pihole.toml and dhcp.leases are affected.
Minor
src/install_paths.h: the header comment says "Compile-time installation path prototypes", which is copy-paste from a function-prototype header, as this file declares no prototypes. "Compile-time installation paths" fits better.src/config/dnsmasq_config.c: line 248 now emitsGLOBALTOMLPATH, but a few lines down the same generated header still hardcodes"...in /etc/pihole/pihole.toml)". The partial change leaves the generateddnsmasq.confheader inconsistent on a relocated build, so both should useGLOBALTOMLPATH.src/args.c: the--gen-x509help examples still print/etc/pihole/tls.pem, which is now inconsistent with the relocated default we show forwebserver.tls.cert. Low priority, but worth aligning here.- The
/etc/piholementions left in descriptive help and comment strings (thecli_pwhelp, the inotify debug description, the config-backup comment) are prose and fine to leave.
Tests
Nothing in CI exercises a non-default PIHOLE_INSTALL_DIR, which is exactly the configuration where the teleporter gap shows up. A minimal relocated build plus a Teleporter export/import round-trip would catch it, worth adding once the contract above is settled.
Naming
PIHOLE_INSTALL_DIR reads fine. Since the value is the config and state root rather than a general install prefix (the log dir lives elsewhere by design), PIHOLE_CONFIG_DIR would be marginally more precise, but we do not feel strongly either way.
|
Wouldn't also introducing an install prefix (+ support for it on the scripts side) be good here? This would open the possibilities for custom installs/packaging a bit more. One could simple relocate it under new prefix: Or change locations of specifics only: I've been maintaining Entware package of Pi-hole for a while now and I simply patch all the paths so this PR has caught my interest as it can make my work a bit easier. Just dropping an idea here... |
|
Compared to jacklul, I am doing some RPM packaging, but I am also patching many of the paths. of course they are not that complicated to patch, but less downstream patches are always preffered. For FHS the following variables may be useful:
How I imagine paths to be mapped using those variables
Compatibilty with cyberb's and jacklul's ideas:
This may be a bit more involved as the core scripts do not have a compilation/build step and are instead used directly. DL6ER raised the concern of teleporter incompatibilities, it should always use the default Again just some thoughts, open to some suggestions to fit you packaging needs. |
|
Two of the open points were ours to decide. Teleporter. Archives should stay portable across installs, so the canonical mz_zip_writer_add_file(zip, "etc/pihole/pihole.toml", GLOBALTOMLPATH, ...);
mz_zip_writer_add_file(zip, "etc/pihole/dhcp.leases", DHCPLEASESFILE, ...);Both names currently come from Scope. We would like to take Remaining items, all small:
Please also rebase - the failing checks are |
…LE_INSTALL_DIR)
All of Pi-hole's configuration and state currently lives under a hardcoded
/etc/pihole. Packagers that install FTL into a different prefix (snaps, /opt
installs, read-only-root images, ...) have to patch the sources.
Introduce a single compile-time base directory, PIHOLE_INSTALL_DIR, defaulting
to /etc/pihole so behaviour is unchanged, and route the /etc/pihole paths
through it: the hardcoded config/state defines (pihole.toml, the legacy config,
dnsmasq.conf, hosts, custom.list, dhcp.leases, backups, cli_pw, versions, the
inotify watch dir), the embedded dnsmasq CONFFILE, and the /etc/pihole defaults
of the runtime-settable config options (files.database, files.tmp_db,
files.gravity, files.macvendor, webserver.tls.cert). Packagers can now relocate
the tree with
cmake -DPIHOLE_INSTALL_DIR=/opt/pihole/etc ...
This is a build-time knob only and deliberately does not add a runtime override
(files.pid stays fixed per GHSA-6w8x-p785-6pm4). The /var/log/pihole log
defaults are left untouched as they are a different base directory.
Teleporter archives stay portable across installs, so the archive entry names
for pihole.toml and dhcp.leases remain the canonical etc/pihole/... ones
regardless of PIHOLE_INSTALL_DIR - only the on-disk source path is relocated.
Export previously derived both names from the absolute path minus its leading
slash, which would have made a relocated build write opt/pihole/etc/... while
the import side still matched the fixed names, silently skipping config and
lease import. Both sides now share ZIPNAME_TOML/ZIPNAME_DHCPLEASES, and a test
asserts the exported archive uses those names.
The embedded OpenAPI specs are run through configure_file() so the five
relocatable defaults they document (cert, database, tmp_db, gravity, macvendor)
and the two adlists.list mentions in lists.yaml follow the configured value
instead of documenting paths a relocated install does not use.
CMake rejects a relative or trailing-slash PIHOLE_INSTALL_DIR, as the teleporter
strips the leading slash by offset.
Signed-off-by: Boris Rybalkin <support@syncloud.it>
10de901 to
91d220e
Compare
|
Rebased onto current Teleporter: took the fixed-name option — All five minor items done: specs ( Kept the |
4 participants
What
Introduce a single compile-time base directory,
PIHOLE_INSTALL_DIR(default/etc/pihole), and route the hardcoded configuration/state paths through it:pihole.toml, the legacy config,dnsmasq.conf, the hosts dir,custom.list,dhcp.leases, backups,cli_pw,versions, the inotify watch dir, and the embedded dnsmasqCONFFILE.Packagers can now relocate the whole tree at configure time:
Why
Everything currently assumes a hardcoded
/etc/pihole. Packages that install FTL under a different prefix - snaps,/optinstalls, read-only-root or otherwise confined images - have to patch the sources. A single overridable base directory removes that need.Notes
files.database,files.log.*) keep their existing config keys, andfiles.pidstays fixed per GHSA-6w8x-p785-6pm4.#ifndefguard in the newsrc/install_paths.h, and the top-levelCMakeLists.txtexposes it as a cache variable + global compile definition so it also reaches the vendored dnsmasqCONFFILE.Happy to adjust the naming (
PIHOLE_INSTALL_DIRvsPIHOLE_ETC_DIR/PIHOLE_CONFIG_DIR) to your preference.