Add Makefile #221
Add Makefile #221
Conversation
| install: | ||
| @# config | ||
| install -d $(DESTDIR)$(sysconfdir)/icinga-notifications | ||
| install -m644 config.example.yml $(DESTDIR)$(sysconfdir)/icinga-notifications/config.yml |
There was a problem hiding this comment.
Why changing this to 644? We use 640 literally everywhere.
There was a problem hiding this comment.
I expect the permissions to still be taken care of by the packaging as the owner has to be set there anyways (and probably isn't even known until installation time due do dynamically allocated user/group IDs). I wouldn't have given -m at all (like I did for the install -d above), but that would result in the file being executable.
There was a problem hiding this comment.
Isn't this Makefile supposed to simplify the Debian rules file? If this file doesn't take care of permissions handling, as we already do in override_dh_auto_install, I don't see how this is supposed to make things simpler, other than to de-duplicate the go build .. parts, for which by the way you don't use the same build flags as we do for the package builds.
There was a problem hiding this comment.
Isn't this Makefile supposed to simplify the Debian rules file? [...] I don't see how this is supposed to make things simpler, other than to de-duplicate the
go build ..parts
It takes care of which files to build an where to install them.
So it should be possible to remove the override_dh_auto_build and override_dh_auto_install targets from debian/rules (those should call make/make install automatically) and then simply set the permissions in override_dh_fixperms (I'm not sure if debian/icinga-notifications is the correct prefix for the path there):
override_dh_fixperms:
dh_fixperms
chmod -R o= debian/icinga-notifications/etc/icinga-notificationsfor which by the way you don't use the same build flags as we do for the package builds.
These can be set using an environment variable: GOFLAGS='-buildvcs=false -trimpath'
We could consider always adding -trimpath, but I wouldn't add -buildvcs=false to the Docker image builds as these are built from a Git checkout and thus receive the proper information that way.
| @# dameon | ||
| install -D build/icinga-notifications $(DESTDIR)$(sbindir)/icinga-notifications | ||
| @# channel plugins | ||
| install -d $(DESTDIR)$(libexecdir)/icinga-notifications/channel |
There was a problem hiding this comment.
The name channel does not make sense to me here, we will not have only one channel there, so I have already adapted this to channels in the Debian rules.
There was a problem hiding this comment.
That has already come up in #210 (comment) as well. Simply using a different path here would introduce an inconsistency with the default path used in the binary:
So if that's supposed to be changed, that's something for its own PR. I don't care too much if it's singular or plural, I wouldn't say any is more or less correct than the other.
Makefile
Outdated
| @# database schema | ||
| install -d $(DESTDIR)$(datadir)/icinga-notifications | ||
| cp -r --no-dereference -v schema $(DESTDIR)$(datadir)/icinga-notifications | ||
| chmod -R u=rwX,go=rX $(DESTDIR)$(datadir)/icinga-notifications |
There was a problem hiding this comment.
Same here as well, why do you need to grant others access to that directory?
There was a problem hiding this comment.
There is no reason why this shouldn't be world readable. These files are coming straight from the package, anyone could download them anyways. The explicit chmod is there to ensure consistent permissions even if a more restrictive umask 077 is set during the build as cp has no option to specify the desired permission and install has no recursive mode.
julianbrost
force-pushed
the
makefile
branch
3 times, most recently
from
6fc67d4 to
9478de6
Compare
It's a very basic Makefile providing just the all/test/install/clean targets intended to avoid duplication of the build and installation steps in the Dockerfile and the rpm/deb packaging repos. /usr/sbin was chosen for the installation path because that's where the Icinga DB packages place the binary. By using the Makefile, the Docker image will now also include the default config and the schema files.
This allows building packages with different values of libexecdir with the
daemon binary automatically adapting the channel plugin dir to it, i.e. so
there is no need to specify it in the config file.
This is relevant for Suse where %{_libexecdir} expands to /usr/lib instead.
Previously, the path to the config file had to be passed on the command line. This commit adds a sensible default so that this is no longer necessary. As the mechanism for setting the channel plugin dir based on libexecdir from the Makefile is already there, the same was used for sysconfdir, even though that should always be /etc for our packaging needs.
There was a problem hiding this comment.
Usage log
$ export DESTDIR=out prefix=/lusr sysconfdir=/aaaa
$ make
mkdir -p build
go build \
-o build/ \
-ldflags "-X 'github.com/icinga/icinga-notifications/internal.LibExecDir=/lusr/libexec' -X 'github.com/icinga/icinga-notifications/internal.SysConfDir=/aaaa'" \
./cmd/icinga-notifications
go build -o build/channel/ ./cmd/channel/...
$ make install
install -d out/aaaa/icinga-notifications
install -m644 config.example.yml out/aaaa/icinga-notifications/config.yml
install -D build/icinga-notifications out/lusr/sbin/icinga-notifications
install -d out/lusr/libexec/icinga-notifications/channel
install build/channel/* out/lusr/libexec/icinga-notifications/channel/
install -d out/lusr/share/icinga-notifications
cp -rv --no-dereference schema out/lusr/share/icinga-notifications
'schema' -> 'out/lusr/share/icinga-notifications/schema'
'schema/pgsql' -> 'out/lusr/share/icinga-notifications/schema/pgsql'
'schema/pgsql/upgrades' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades'
'schema/pgsql/upgrades/001.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/001.sql'
'schema/pgsql/upgrades/002.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/002.sql'
'schema/pgsql/upgrades/003.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/003.sql'
'schema/pgsql/upgrades/004.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/004.sql'
'schema/pgsql/upgrades/005.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/005.sql'
'schema/pgsql/upgrades/006.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/006.sql'
'schema/pgsql/upgrades/007.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/007.sql'
'schema/pgsql/upgrades/008.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/008.sql'
'schema/pgsql/upgrades/009.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/009.sql'
'schema/pgsql/upgrades/010.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/010.sql'
'schema/pgsql/upgrades/011.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/011.sql'
'schema/pgsql/upgrades/012.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/012.sql'
'schema/pgsql/upgrades/013.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/013.sql'
'schema/pgsql/upgrades/014.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/014.sql'
'schema/pgsql/upgrades/015.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/015.sql'
'schema/pgsql/upgrades/016.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/016.sql'
'schema/pgsql/upgrades/017.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/017.sql'
'schema/pgsql/upgrades/018.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/018.sql'
'schema/pgsql/upgrades/019.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/019.sql'
'schema/pgsql/upgrades/020.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/020.sql'
'schema/pgsql/upgrades/021.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/021.sql'
'schema/pgsql/upgrades/022.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/022.sql'
'schema/pgsql/upgrades/023.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/023.sql'
'schema/pgsql/upgrades/024.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/024.sql'
'schema/pgsql/upgrades/025.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/025.sql'
'schema/pgsql/upgrades/026.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/026.sql'
'schema/pgsql/upgrades/027.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/027.sql'
'schema/pgsql/upgrades/028.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/028.sql'
'schema/pgsql/upgrades/029.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/029.sql'
'schema/pgsql/upgrades/030.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/upgrades/030.sql'
'schema/pgsql/schema.sql' -> 'out/lusr/share/icinga-notifications/schema/pgsql/schema.sql'
chmod -R u=rwX,go=rX out/lusr/share/icinga-notifications/schema
$ ./out/lusr/sbin/icinga-notifications -version
Icinga Notifications version: 0.0.0-g9ad4cbc-dirty
Build information:
Go version: go1.22.3 (linux, amd64)
Git commit: 9ad4cbcc5cc4964663e573d7dedee4d57a6a059b (modified)
$ make clean
rm -rf build
It's a very basic Makefile providing just the
all/test/install/cleantargets intended to avoid duplication of the build and installation steps in the Dockerfile and the rpm/deb packaging repos./usr/sbinwas chosen for the installation path because that's where the Icinga DB packages place the binary.By using the Makefile, the Docker image will now also include the default config and the schema files.
Additionally, the Makefile sets two Go variables to the values of
libexecdirandsysconfdirusinggo build -ldflags '-X ...'. This allows the daemon to use these values for the channel plugin dir and the default config path (which had no default so far and is added by this PR). Adding this mechanism is necessary for building proper packages for Suse where%{_libexecdir}expands to/usr/lib.