-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dist: build debian packages for multiple platforms #4448
Conversation
9f50bf9
to
46127f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 26 of 26 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @marcfrei, @matzf, and @oncilla)
Makefile
line 22 at r1 (raw file):
if [ -f "$$f" ]; then \ bf=`basename $$f`; \ v="$$(ar p $$f control.tar.gz | tar -xz --to-stdout ./control | sed -n 's/Version: //p')"; \
What's control.tar.gz? Is that the control service? Why just this one? I don't think I understand what this is about.
dist/BUILD.bazel
line 64 at r1 (raw file):
"adduser", ], description = "SCION dispatcher",
Was that meant to read "SCION daemon"?
Code quote:
description = "SCION dispatcher",
dist/debian/scion.postinst
line 19 at r1 (raw file):
configure) # Create system user adduser --system --home /var/lib/scion --group scion
If this is ever executed redundantly, it will give you an error.
Add || true?
Did you mean "--user-group" rather than "--group" ? "--group" would be the abbreviation of "--groups", which would consume the user name argument that follows and also expect the group by that name to exists.
Code quote:
adduser --system --home /var/lib/scion --group scion
dist/systemd/[email protected]
line 16 at r1 (raw file):
[Install] WantedBy=multi-user.target
What happens to the OS if this fails? Does it not fail until someone creates the config, btw? Should we arrange for some kind of dormant state for the router, by way of some default/empty config file?
Code quote:
multi-user.target
doc/dev/setup.rst
line 120 at r1 (raw file):
.. code-block:: bash go build -o bin ./<service>/cmd/<service>...
Did you delete one of the two sqlite implementation? I didn't see that.
Code quote:
go build -o bin ./<service>/cmd/<service>...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @marcfrei, @matzf, and @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 25 of 26 files reviewed, 5 unresolved discussions (waiting on @jiceatscion, @marcfrei, @matzf, and @oncilla)
Makefile
line 22 at r1 (raw file):
Previously, jiceatscion wrote…
What's control.tar.gz? Is that the control service? Why just this one? I don't think I understand what this is about.
Ah, no unrelated to our control service. This is the manifest of the .deb package, see https://en.wikipedia.org/wiki/Deb_(file_format)#Control_archive.
Added bit better explanation and a reference to the wikipedia page (which gives a much more concise overview than the official spec).
dist/BUILD.bazel
line 64 at r1 (raw file):
Previously, jiceatscion wrote…
Was that meant to read "SCION daemon"?
Done.
dist/debian/scion.postinst
line 19 at r1 (raw file):
Previously, jiceatscion wrote…
If this is ever executed redundantly, it will give you an error.
Add || true?
Did you mean "--user-group" rather than "--group" ? "--group" would be the abbreviation of "--groups", which would consume the user name argument that follows and also expect the group by that name to exists.
It prints an error message if the user already exists (which I do find a bit annoying) , but it still succeeds. As I understand, this is by design; this is following the advice here: https://wiki.debian.org/AccountHandlingInMaintainerScripts
Regarding --groups vs --user-group; is it possible that your looking useradd
instead of adduser
? There doesn't seem to be such a flag, at least not in debian, see https://manpages.debian.org/jessie/adduser/adduser.8.en.html
dist/systemd/[email protected]
line 16 at r1 (raw file):
Previously, jiceatscion wrote…
What happens to the OS if this fails? Does it not fail until someone creates the config, btw? Should we arrange for some kind of dormant state for the router, by way of some default/empty config file?
This [Install] section only applies when a user enables the service. For example systemctl enable [email protected]
. This does not happen during package installation. Nothing bad happens if this fails. It just means that the systemd (attempts to) start the service whenever this multi-user target is activated. If it fails, it's marked as failing, but nothing else happens. This multi-user target is pretty much the same as runlevel 5.
Btw. it seems I need to write some documentation on how to use this stuff.
doc/dev/setup.rst
line 120 at r1 (raw file):
Previously, jiceatscion wrote…
Did you delete one of the two sqlite implementation? I didn't see that.
Ah no sorry, this is an incomplete edit. I wanted to remove the explicit tag here to simplify the instructions, because there is a default option. (It' I wanted to move the documentation of the two options to somewhere else under "advanced build instructions", but I didn't find the right place for it yet. I'll add it somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marcfrei, @matzf, and @oncilla)
dist/debian/scion.postinst
line 19 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
It prints an error message if the user already exists (which I do find a bit annoying) , but it still succeeds. As I understand, this is by design; this is following the advice here: https://wiki.debian.org/AccountHandlingInMaintainerScripts
Regarding --groups vs --user-group; is it possible that your looking
useradd
instead ofadduser
? There doesn't seem to be such a flag, at least not in debian, see https://manpages.debian.org/jessie/adduser/adduser.8.en.html
Indeed. On my system adduser is a symlink to useradd! Ditto for the manpage. Ok, mine is redhat/fedora. Your stuff is for debian/ubuntu. So... objection withdrawn.
dist/systemd/[email protected]
line 16 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
This [Install] section only applies when a user enables the service. For example
systemctl enable [email protected]
. This does not happen during package installation. Nothing bad happens if this fails. It just means that the systemd (attempts to) start the service whenever this multi-user target is activated. If it fails, it's marked as failing, but nothing else happens. This multi-user target is pretty much the same as runlevel 5.Btw. it seems I need to write some documentation on how to use this stuff.
Actually, according to debian's doc it is more like runlevel 3. They have another name for runlevel 5 ("graphics", I think). Anyway. As long as it doesn't constantly retry, I have no objection. Esp. If it begins life disabled (I hadn't considered that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marcfrei, @matzf, and @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 23 of 26 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf and @oncilla)
dist/conffiles/sciond.toml
line 13 at r2 (raw file):
# Optionally enable DRKey # [drkey_db]
This is the old DRKey config, right? Not yet using drkey_level2_db
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 25 of 26 files reviewed, 2 unresolved discussions (waiting on @jiceatscion, @matzf, and @oncilla)
dist/conffiles/sciond.toml
line 13 at r2 (raw file):
Previously, marcfrei (Marc Frei) wrote…
This is the old DRKey config, right? Not yet using
drkey_level2_db
.
Thanks for the fix, Matthias!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 23 of 26 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @matzf and @oncilla)
Makefile
line 17 at r3 (raw file):
bazel build //dist:deb_all mkdir -p deb; rm -f deb/*; @ # Bazel cannot include the version in the filename.
huh? I thought usually it includes the version in the filename? Maybe the comment is just a bit misleading?
README.md
line 49 at r3 (raw file):
operational global test deployment of SCION. The [awesome-scion](https://github.com/scionproto/awesome-scion#deployments) list containes
Suggestion:
contains
dist/systemd/[email protected]
line 13 at r3 (raw file):
User=scion Group=scion ExecStart=/usr/bin/scion-control --config /etc/scion/%i.toml
what is the argument? The ISD-AS? Shouldn't the daemon also have that?
dist/test/BUILD.bazel
line 10 at r3 (raw file):
env = { "SCION_DEB_PACKAGES": "$(locations //dist:deb)", "DEBUG": "1",
do we want to keep the debug env here? doesn't that leak containers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 23 of 29 files reviewed, 5 unresolved discussions (waiting on @jiceatscion, @lukedirtwalker, @marcfrei, and @oncilla)
Makefile
line 17 at r3 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
huh? I thought usually it includes the version in the filename? Maybe the comment is just a bit misleading?
It doesn't work if we want to use the git tag as the version. We have two options: either write a the version to a build file before building, or rename the files after building. I chose the latter option, because we can get the version into the .deb package just fine, just not into the filename.
Made the comment more specific. Better?
README.md
line 49 at r3 (raw file):
operational global test deployment of SCION. The [awesome-scion](https://github.com/scionproto/awesome-scion#deployments) list containes
Done.
dist/conffiles/sciond.toml
line 13 at r2 (raw file):
Previously, marcfrei (Marc Frei) wrote…
This is the old DRKey config, right? Not yet using
drkey_level2_db
.
Good catch, fixed.
dist/systemd/[email protected]
line 13 at r3 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
what is the argument? The ISD-AS? Shouldn't the daemon also have that?
I've added documentation for this now (see doc/manuals/install.rst). The parameter is the basename of the config file.
For the control service and the router we support service instances per host, e.g. for operating multiple ASes (or same AS in different ISDs) from the same host.
For the daemon running multiple instances is less useful; you'd need to specify the daemon address for every application invocation, and that just seems too cumbersome.
dist/test/BUILD.bazel
line 10 at r3 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
do we want to keep the debug env here? doesn't that leak containers?
Ooops, fixed.
doc/dev/setup.rst
line 120 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ah no sorry, this is an incomplete edit. I wanted to remove the explicit tag here to simplify the instructions, because there is a default option. (It' I wanted to move the documentation of the two options to somewhere else under "advanced build instructions", but I didn't find the right place for it yet. I'll add it somewhere.
Done. I've added documentation on installation and related to this reorganized the build instructions a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 6 files at r4.
Reviewable status: 28 of 29 files reviewed, 3 unresolved discussions (waiting on @jiceatscion, @matzf, and @oncilla)
Makefile
line 17 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
It doesn't work if we want to use the git tag as the version. We have two options: either write a the version to a build file before building, or rename the files after building. I chose the latter option, because we can get the version into the .deb package just fine, just not into the filename.
Made the comment more specific. Better?
Aha, I see 👍
README.md
line 49 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
Done.
don't see it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 6 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @oncilla)
dist/systemd/[email protected]
line 13 at r3 (raw file):
Previously, matzf (Matthias Frei) wrote…
I've added documentation for this now (see doc/manuals/install.rst). The parameter is the basename of the config file.
For the control service and the router we support service instances per host, e.g. for operating multiple ASes (or same AS in different ISDs) from the same host.
For the daemon running multiple instances is less useful; you'd need to specify the daemon address for every application invocation, and that just seems too cumbersome.
Well if you really operate multiple ISD-AS and want to use the scion tooling (like showpaths) you need to have multiple daemon instances. Using the env file you can specify the daemon address in the env config file, and you would only need the ISD-AS paramater on scion tooling. Anyway just putting this out here. Not really needed to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @oncilla)
README.md
line 49 at r3 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
don't see it?
Sorry, didn't push, now done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf and @oncilla)
doc/dev/setup.rst
line 120 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Done. I've added documentation on installation and related to this reorganized the build instructions a bit.
Sounds good, I don't see where the documentation regarding the two sql options has moved, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)
doc/dev/setup.rst
line 120 at r1 (raw file):
Previously, jiceatscion wrote…
Sounds good, I don't see where the documentation regarding the two sql options has moved, though.
doc/dev/build.rst (sub-section "Options").
Never mind. I found it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 20 of 26 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, 6 of 6 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matzf)
Instead of forcing building for all platforms, determine the architecture from the current build platform. Packages can now be built for individual platforms, as long as the architecture "select" statement works. Add a filegroup wrapper to still build for a predefined list of target platforms.
Just a "ci-fixed" will be invalid in the debian packages.
This reverts commit 012dd9c83180f56c464bae5879e4aab1e3ffb7f1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @marcfrei)
doc/dev/build.rst
line 73 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Prerequisites
Done.
doc/dev/build.rst
line 83 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Bazel
Done.
doc/dev/build.rst
line 90 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Bazel
Done.
doc/dev/build.rst
line 91 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
requires
Done.
doc/dev/build.rst
line 91 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
as
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matzf)
Build debian packages for amd64, arm64, i386 and armel. - There are separate packages for router, control, daemon, dispatcher, gateway and tools (scion and scion-pki). - The packages include systemd unit files to run the services. For the daemon, dispatcher and gateway one instance per host is supported by the systemd service, and a default configuration file is included. For router and control, multiple instances per host are supported, and as a consequence of this, no default configuration file is provided. - Currently, there is no man page contained in the packages. We should be able to build these from our existing manuals, but it seems to require a significant amount of fiddling to get something useful. Building the .deb packages uses bazel with `rules_pkg`. The target `//dist:deb_all` cross-builds packages for the default set of target platforms. Alternatively, the target `//dist:deb` allows to build (all) packages for the current target platform. This current platform can be set with the `--platforms` bazel option (see https://github.com/bazelbuild/rules_go#how-do-i-cross-compile for more details). - To increase reuse of build results while cross-building, some internal targets related to openapi forcibly ignore the target platform - The package version is based on the current git tag. `rules_pkg` can include this version _in_ the package metadata, but bazel _cannot_ spit out appropriately named package files. This is addressed by copying and renaming the package files after build in a make target `make dist-deb`. Add installation documentation for the packages and the systemd units. As a side effect, slightly reorganize the build documentation also, trying to make give a simpler path to just build the binaries without installing the entire development setup.
Build debian packages for amd64, arm64, i386 and armel.
Building the .deb packages uses bazel with
rules_pkg
.The target
//dist:deb_all
cross-builds packages for the default set of target platforms. Alternatively, the target//dist:deb
allows to build (all) packages for the current target platform. This current platform can be set with the--platforms
bazel option (see https://github.com/bazelbuild/rules_go#how-do-i-cross-compile for more details).rules_pkg
can include this version in the package metadata, but bazel cannot spit out appropriately named package files. This is addressed by copying and renaming the package files after build in a make targetmake dist-deb
.Contributes to #4425.
This change is