Skip to content
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

Feature/debian installer #917

Merged
merged 28 commits into from
Oct 19, 2021
Merged

Feature/debian installer #917

merged 28 commits into from
Oct 19, 2021

Conversation

ersonp
Copy link
Contributor

@ersonp ersonp commented Oct 8, 2021

Did you run make format && make check? yes

Fixes #

Changes:

  • Added debian package builder script and docs
  • Added debian package sigining script and docs

How to test this PR:

  1. Follow the deb_install.md

The commit contains the scripts to build debian package of skywire and installation via apt-get.
The commit contains the documentation for the debian package building and installation.
The commit removes unwanted scripts.
The commit contains the updated package_deb.sh and some fixes.
This commit contains updated docs for deb_installer.
The commit contains additions in gitignore.
The commit contains the fix for indentation of package_deb.sh.
The commit contains a minor fix. A new line is added at the end of sign_deb.sh.
The commit contains makefile targets for deb-install-prequisites, deb-package and deb-package-help. It also includes the changes and additions necessary for the targets and some minor fixes.
The commit contains updates to the deb_install docs.
The commit contains changes to the input type of package_deb.sh, instead of taking the version, email and name via flag it now takes it from the console.
The commit contains updates to the docs.
The commit contains updates to the docs.
The commit contains sign_deb.sh.
The commit contains updates to the docs.
@ersonp ersonp marked this pull request as ready for review October 8, 2021 17:26
Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure we build the binaries we package/sign with the debian build tag. That should be either part of the target or clear part of the instrucitons.

The commit contains update to the package_deb.sh where instead of using make bin and make host-apps seperatly we now use make build with build tag. It also includes some minor changes.
The commit contains additions to teh doc. The additions are install and uninstall instructions for manually built packages and uninstall instructions for end use.
Copy link
Contributor

@alexadhy alexadhy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes needed, I think for hygiene. Not crucial at all, but would be nice to have.

scripts/debian_installer/package_deb.sh Outdated Show resolved Hide resolved
scripts/debian_installer/package_deb.sh Outdated Show resolved Hide resolved
The commit contains changes to the package_deb.sh where files were written with echo are now written with cat.
The commit contains changes to the files sign_deb.sh and prequisites.sh where empty line was added to their end.
@0pcom
Copy link
Collaborator

0pcom commented Oct 15, 2021

For the package installed to the path /opt/skywire; the config gen command (for a visor) should be

skywire-cli config gen -pro /opt/skywire/skywire-visor.json

without this flag, the current directory is written into the config files; which may be /

the binaries should be in /opt/skywire/bin/

the app are in the correct place

The config gen stuff brings up an important question in terms of what defaults are desired.

In the packages I maintain of skywire; a hypervisor instance is configured by the postinstall script with the command:

skywire-cli config gen -ipr

which outputs by default to the path /opt/skywire/skywire.json

the hypervisor instance is enabled and started by default upon installation by the same postinstall script.

the postinstall script also prints out the visor's public key and the ip address and port of the running hypervisor instance

It is left to the user, at that point, to configure additional visor instances to use that hypervisor instance by running a command that is printed to the terminal.

cd /opt/skywire
sudo cp skywire.json skywire-visor.json
sudo skywire-cli visor gen-config --hypervisor-pks <hypervisor-public-key> -pro skywire-visor.json
sudo systemctl disable --now skywire.service && sudo systemctl enable --now skywire-visor.service

cd /opt/skywire is needed because I think even with the -p flag there are some fields in the config file which may use the current dir.

In the packages i maintain; there are two systemd service files. One for hypervisor and one for visor-only. The process outlined keeps the same keys for both configs.

To aid in troubleshooting; the postinstall script of the packages is made to call a script which is included with the installation; skywire-autoconfig which is designed to restore a correct running state to the visor if the config is accidentally deleted or corrupted. This script determines whether to run a visor or hypervisor instance based on the presence of the visor config file.

@jdknives I suggest comparing the packages I maintain with the ones generated by this PR.

the repository configuration instructions are at:

fiber.magnetosphere.net

Also, I will point out that I don't believe apt does signature checking of packages by default. but the signature on the repository is checked.


This manner of doing a build of a .deb package is a heavily one-off effort which is difficult to maintain and very difficult for others, myself included, to understand what is going on and troubleshoot. Because the build system occupies the same script as the build itself; and also the way the different files are generated and written to is a bit tough on the eyes. I certainly am not discounting the effort here.

Compare with this cross-compiling .deb PKGBUILD. The skywire-autoconfig script i mentioned earlier can be found in the scripts archives when you git clone https://aur.archlinux.org/skywire-bin

The constraint with this build system is that an archlinux host or chroot is required.

Additionally, I maintain packages in the AUR for several skycoin repositories, and already have .deb builds for some; but the process merely adding a few lines to the existing PKGBUILD.

the PKGBUILD format is much simpler and easier to read. It is also not designed to accompany the sources as these are fetched automatically by makepkg

it is actually possible at present for anyone to run the following commands on archlinux to create the skywire debian packages:

yay -S skywire-bin
cd $HOME/.cache/yay/skywire-bin
makepkg -p cc.deb.PKGBUILD

Or it would have been possible if the 0.4.2 release of skywire had not been deleted. Updating the version is a trivial matter.

@jdknives
Copy link
Member

@ersonp please verify and fix the issues from the following passage

For the package installed to the path /opt/skywire; the config gen command (for a visor) should be
skywire-cli config gen -pro /opt/skywire/skywire-visor.json

@0pcom 0pcom mentioned this pull request Oct 15, 2021
@jdknives
Copy link
Member

@0pcom

the binaries should be in /opt/skywire/bin/

Why? That diverges from the layout we use for source builds.

The config gen stuff brings up an important question in terms of what defaults are desired.

Which questions does that bring up exactly? Not sure I follow.

the postinstall script also prints out the visor's public key and the ip address and port of the running hypervisor instance

Not required. The Deb package will be geared towards VPN users. This includes a Systray application that allows you to open hypervisor UI and VPN UI easily from the systray. If a user needs the PubKey of the visor, it is easy enough to copy from the UI.

In the packages i maintain; there are two systemd service files.

Not required. This package is not intended for system administrators or network administrators. It is geared towards users interested in the VPN. Therefore it does not need to support visor mode and can just assume users wants UI. People who want to setup clusters can either use the existing Skyimager or can do/build their own scripts. They are going to be a minority of cases and I do not see a need to maintain a package for this specific use case.

I dont see the need for the autoconfig command or script at this point. It is reasonably hard to accidentally remove the config. If this happens, user can reinstall.

This manner of doing a build of a .deb package is a heavily one-off effort which is difficult to maintain

Why? Please expand.

to understand what is going on and troubleshoot

Can have a look at improvements.

Building the deb packages from PKGBUILD is something that may be beneficial. But we already have this here and I dont think it very urgent to optimize the package building process too much at this time as it happens once every three months.

scripts/deb_installer/package_deb.sh Outdated Show resolved Hide resolved
scripts/deb_installer/package_deb.sh Outdated Show resolved Hide resolved
scripts/deb_installer/package_deb.sh Outdated Show resolved Hide resolved
@0pcom
Copy link
Collaborator

0pcom commented Oct 16, 2021

@0pcom

the binaries should be in /opt/skywire/bin/

Why? That diverges from the layout we use for source builds.

if using the -p flag for config generation, all the folders that are created by the running visor instance will populate in the same directory as those binaries.

The config gen stuff brings up an important question in terms of what defaults are desired.

Which questions does that bring up exactly? Not sure I follow.

My mistake, I did not realize that this was geared towards the end vpn users because most or all of the skywire installation is present.

The commit contains requested changes. The Makefile comment is cleaned up as well as the makefile content is moved to it's own file which is copied over to the deb packages at the time of packaging. The same is done with the heredoc strings.
Step for config generation has been added in the docs.
Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better.

@jdknives jdknives merged commit 537d38c into skycoin:develop Oct 19, 2021
@ersonp ersonp deleted the feature/debian-installer branch April 11, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants