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

Add FirebirdSQL server #5837

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

vitkabele
Copy link
Contributor

@vitkabele vitkabele commented Aug 9, 2023

Description

Hello,
I created the spk/firebird package for the FirebirdSQL server. The cross compilation works, package can be installed.

As part of this merge request, I also added native/libtool as this is required for building libtom(math|crypt) libraries during the Firebird build.

The cross/daemon utility is compiled because the firebird server cannot start and fork itself as daemon. (Can if started via fbguard binary.

The package provides UI wizard that allows user to choose a new password for the SYSDBA main system user.
Started service is available on the port 3050.

The package successfully compiles for armv8, armv7 and x64 architectures. The installation was tested on my DS215j with armv7.

What must be done before the package can be released:

  • Fix the linkage problems during server startup.
  • Create security4.fdb database during package install (this requires copying security4.gbak to package)
  • Create firebird.msg file during install (msg.gbak -> msg.fdb -> build_file firebird.msg). This requires having build_file binary in the package
  • Create SYSDBA database user during package install.

This pull request is followup from the #5795 issue as most of the work is done and the reviews can start.
Please also try to install the package on different architectures and report eventual bugs to me.

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

vitkabele added a commit to vitkabele/spksrc that referenced this pull request Aug 11, 2023
vitkabele added a commit to vitkabele/spksrc that referenced this pull request Aug 11, 2023
Build FirebirdSQL database

The build works for x64 and aarch64 architecture and DSM 7.0 and 7.1

- Use configure --with-cross-build as used by firebird build itself
vitkabele added a commit to vitkabele/spksrc that referenced this pull request Aug 11, 2023
@vitkabele vitkabele marked this pull request as ready for review August 11, 2023 18:31
vitkabele added a commit to vitkabele/spksrc that referenced this pull request Aug 11, 2023
Build FirebirdSQL database

The build works for x64 and aarch64 architecture and DSM 7.0 and 7.1

- Use configure --with-cross-build as used by firebird build itself
vitkabele added a commit to vitkabele/spksrc that referenced this pull request Aug 11, 2023
Build FirebirdSQL database

The build works for x64, armv7 and aarch64 architectures with DSM 7.0 and 7.1

- Use configure --with-cross-build as used by firebird build itself
Create spk package definition with service configuration and package
icon. Also provide UI install wizard to allow the user choose the
password for the SYSDBA user.
cross/firebird/Makefile Outdated Show resolved Hide resolved
@hgy59
Copy link
Contributor

hgy59 commented Aug 11, 2023

@vitkabele thanks for your contribution. This is a lot of customization.

Just added my first impression. I think the most important is to validate a package update does not loose any custom data.

spk/firebird/Makefile Outdated Show resolved Hide resolved
spk/firebird/Makefile Outdated Show resolved Hide resolved
Copy link
Member

@publicarray publicarray left a comment

Choose a reason for hiding this comment

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

I can't comment on the package it self atm but I've found some small things that we typically do.

cross/firebird/PLIST Outdated Show resolved Hide resolved
spk/firebird/src/wizard/install_uifile Outdated Show resolved Hide resolved
vitkabele and others added 3 commits August 20, 2023 10:43
Declare the FB_* paths in the C(XX)FLAGS in the cross/Makefile instead
of in the cross_config include files. These paths affect the runtime
behavior of Firebird and must match the path on the installed system.

The paths for configuration ($SYNOPKG_PKGVAR) however differ during
install and runtime, as the installer script moves the content of
$PREFIX/var to $SYNOPKG_PKGVAR during installation.

The change of FB_CONFDIR also changes the PREFIX a.k.a. root directory
of the whole Firebird installation and when modified, all other paths
must be explicitly specified.
@vitkabele
Copy link
Contributor Author

vitkabele commented Aug 22, 2023

I went through all the proposals and implemented most of them. You can give it another look. How do you want to do the merge? Should I somehow squash the changes?
I would also like to provide a README.md in the cross/firebird with description of some non-intuitive choices that I had to do when porting the package. I see that no other package has it in there, but I think that it is better place to put this than the github wiki as this will be eventually more useful for us in the future than for the end users of the package.
I also used an existing firebird database and successfully connected to it from a remote client. Although I performed only very basic interactions, there doesn't seem to be any "fatal flaw".

Comment on lines +46 to +63
ADDITIONAL_CFLAGS = '-DFB_PREFIX="\"$(SYNOPKG_PKGDEST)\""' \
'-DFB_SECDBDIR="\"$(SYNOPKG_PKGVAR)\""' \
'-DFB_GUARDDIR="\"$(SYNOPKG_PKGVAR)\""' \
'-DFB_CONFDIR="\"$(SYNOPKG_PKGVAR)\""' \
'-DFB_SBINDIR="\"$(SYNOPKG_PKGDEST)/bin\""' \
'-DFB_BINDIR="\"$(SYNOPKG_PKGDEST)/bin\""' \
'-DFB_DOCDIR="\"$(SYNOPKG_PKGDEST)/doc\""' \
'-DFB_HELPDIR="\"$(SYNOPKG_PKGDEST)/help\""' \
'-DFB_SAMPLEDIR="\"$(SYNOPKG_PKGDEST)/examples\""' \
'-DFB_PLUGDIR="\"$(SYNOPKG_PKGDEST)/plugins\""' \
'-DFB_SAMPLEDBDIR="\"$(SYNOPKG_PKGDEST)/examples/empbuild\""' \
'-DFB_TZDATADIR="\"$(SYNOPKG_PKGDEST)\""' \
'-DFB_MISCDIR="\"$(SYNOPKG_PKGDEST)/misc\""' \
'-DFB_MSGDIR="\"$(SYNOPKG_PKGDEST)\""' \
'-DFB_INCDIR="\"$(SYNOPKG_PKGDEST)/include\""' \
'-DFB_INTLDIR="\"$(SYNOPKG_PKGDEST)/intl\""' \
'-DFB_LIBDIR="\"$(SYNOPKG_PKGDEST)/lib\""' \
'-DFB_LOGDIR="\"$(SYNOPKG_PKGVAR)\""'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a missunderstanding
SYNOPKG_* variables are for installer only (i.e. service-setup.sh), not for building packages

for Makefiles you have to use the Makefile Variables as documented in the wiki
like STAGING_INSTALL_PREFIX

Copy link
Contributor

@hgy59 hgy59 Aug 22, 2023

Choose a reason for hiding this comment

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

If FirebirdSQL follows the known rules, you only have to define FB_PREFIX and other subfolders are defaulted to ${FB_PREFIX}/bin, ${FB_PREFIX}/lib, ...}

you should only require the variables

  • INSTALL_PREFIX
  • INSTALL_PREFIX_VAR

normally you do not define such FB_* variables as compiler flags but as configure options.

the configure arg --prefix=$(INSTALL_PREFIX) should make the definition of ADDITIONAL_CFLAGS += FB_PREFIX=$(INSTALL_PREFIX) obsolete.

NB:
the spksrc framework is designed to use /usr(local/{packagename} as prefix when building a package with the cross Makefile and to use /var/packages/{packagename}/target as prefix when building the package in the spk folder.

Copy link
Contributor Author

@vitkabele vitkabele Aug 23, 2023

Choose a reason for hiding this comment

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

for Makefiles you have to use the Makefile Variables as documented in the wiki
like STAGING_INSTALL_PREFIX

As I understand, the STAGING_INSTALL_PREFIX is the location where the files should be copied to be found by the PLIST magic. What I need here is actually the runtime location of the files, because Firebird compiles this information in the binary. If not set properly, it does not find the configuration etc.

If FirebirdSQL follows the known rules, you only have to define FB_PREFIX and other subfolders are defaulted to ${FB_PREFIX}/bin, ${FB_PREFIX}/lib, ...}

This works to a certain extent. With this setup, configuration files are located in $FB_PREFIX, where the installer does not find them. Even if I move them later to $FB_PREFIX/var, the Firebird itself won't find them during startup and crashes.

normally you do not define such FB_* variables as compiler flags but as configure options.

This is true, but the Firebird's build process is non-standard at least. The server is built in two phases, first some native part is built, this is then used to pre-process certain files and then the actual cross toolchain is used to build the binary.

In this process, the --with-fbprefix and similar configure option have only limited usability, because the value from them does not reach the cross phase for the reason you mention in your next comment. The file that should be generated with autoconf is not generated, which is the destination location of the discussed variables.

There is a cli option for firebird server to change the firebird root on startup, but this unfortunately breaks other things. Please see my related question in firebird-support mailing list.

TL;DR The build process cannot handle when install prefix and actual runtime prefix is different. This is the purpose of this -DFB_* fun.

@hgy59
Copy link
Contributor

hgy59 commented Aug 22, 2023

@vitkabele IMHO it is not worth all the workaround to build this package.
firebirdsql developers should fix the autoconf/configure files for this package to work!

it should work with this preconfigure step and the default configure target of spksrc

GNU_CONFIGURE = 1

PRE_CONFIGURE_TARGET = firebird_pre_configure

.PHONY: firebird_pre_configure
firebird_pre_configure:
	@$(RUN) ./autogen.sh --enable-binreloc-threads --with-builtin-tommath --with-builtin-tomcrypt --prefix=$(INSTALL_PREFIX)

But it even fails to run gcc of the toolchain(s)

Running ./configure --enable-binreloc-threads --with-builtin-tommath --with-builtin-tomcrypt --prefix=/usr/local/firebird ...
checking whether make sets $(MAKE)... yes
checking build system type... x86_64-pc-linux-gnu
checking for /proc/self/maps... yes
checking whether everything is installed to the same prefix... no
checking whether binary relocation support should be enabled... no
checking host system type... x86_64-pc-linux-gnu
checking for gcc... /spksrc/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-gcc
checking whether the C compiler works... no
configure: error: in `/spksrc/cross/firebird_hgy/work-x64-7.1/firebird-4.0.3':
configure: error: C compiler cannot create executables
See `config.log' for more details

just some points I am wondering about

  • you should not need the native/libtool as libtool is available in the dev env (or - if it needs the libtool-dev package you must build libtool for the target ARCH and not native)
  • you should not need to provide the src/cross_config/synology.${ARCH}.h files, respecitve definitions should be created by configure
  • you should not need to provide the src/make.synology.${ARCH} files for the same reason

@vitkabele
Copy link
Contributor Author

vitkabele commented Aug 23, 2023

@vitkabele IMHO it is not worth all the workaround to build this package.

Well I've already spent like two months of my evenings with it now and it is done.

firebirdsql developers should fix the autoconf/configure files for this package to work!

I am afraid this is not so straightforward. Thanks to the two phase build process and the fact that they require some runtime checks from the autoconf, which cannot be performed on build host, when the target machines properties are questioned.

  • you should not need the native/libtool as libtool is available in the dev env (or - if it needs the libtool-dev package you must build libtool for the target ARCH and not native)

I need the libtool in the first phase of the build process, this is for the native environment. What I found in the docker is just libtool package, but I need the libtool binary, which is in the package libtool-bin which is not installed in the environment.

  • you should not need to provide the src/cross_config/synology.${ARCH}.h files, respecitve definitions should be created by configure
  • you should not need to provide the src/make.synology.${ARCH} files for the same reason

This is the way in which firebird itself is cross-built, you can check similar makefile for android-arm64, android-arme and config files for android-arm64 resp. android-arme in their original source tree.
I doubt that Firebird developers will change their whole build process because of it is little inconvenient for us. It seems to work for them for years.

@vitkabele vitkabele requested a review from publicarray August 24, 2023 06:29
@vitkabele vitkabele mentioned this pull request Aug 28, 2023
10 tasks
@vitkabele
Copy link
Contributor Author

So the client who was at the beginning of my journey with this package has successfully installed it on DS718+ and used it in a production environment for a first week. So far the Firebird server seems to run without issues.

@vitkabele
Copy link
Contributor Author

vitkabele commented Sep 9, 2023

Hello,
I won't have time to work on that further in following months. Could we please merge it to the upstream and let the package build in the repository? The installation works reliably.

Comment on lines +74 to +82
if [ "x$wizard_sysdba_password" = "x" ]; then
# User provided new password. We override the password file even
# if it exists because the user expects to have the new password.
gen_password > "$SYSDBA_PASSWORD_FILE";
else
# Use the users password
printf "%s" "$wizard_sysdba_password" > "$SYSDBA_PASSWORD_FILE"
fi
SET_PASSWD=true
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is somewhat missleading.
In the if clause, the user did not provide a password (this is only possible with bypass of the wizard), but the comment is User provided new password.

Comment on lines +97 to +98
echo "Setting 'SYSDBA' password to '${SYSDBA_PASSWORD}'";

Copy link
Contributor

Choose a reason for hiding this comment

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

Please never print plain password in installation log files.

Suggested change
echo "Setting 'SYSDBA' password to '${SYSDBA_PASSWORD}'";
echo "Setting 'SYSDBA' password from '${SYSDBA_PASSWORD_FILE}'";

Comment on lines +84 to +89
if $SECDB_CREATED && [ ! -f "$SYSDBA_PASSWORD_FILE" ]; then
# We are upgrading/re-installing and the user deleted
# both the password file and the database file
gen_password > "$SYSDBA_PASSWORD_FILE";
SET_PASSWD=true
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO SET_PASSWORD=true must always be set when SECDB_CREATED is true.
This must not depend on non existing password file.

Otherwise the usecase "delete security db to recreate it without deletion of the password file" would not work (i.e. the password is not set).
Since upgrade is not allowed with deleting both security db and password file this if clause will never be executed and can be removed (when SET_PASSWD=true is set when creating the security db).

Comment on lines +53 to +54
# We just built the security database. We must also set the password.
SECDB_CREATED=true
Copy link
Contributor

Choose a reason for hiding this comment

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

With this solution the else #UPGRADING block blow (see comment) is fixed can be removed.

Suggested change
# We just built the security database. We must also set the password.
SECDB_CREATED=true
# We just built the security database. We must also set the password.
SECDB_CREATED=true
SET_PASSWD=true

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.

5 participants