euro-office-documentserver: init at 9.3.1#504934
Conversation
3749928 to
c91e0d1
Compare
|
thank you @SuperSandro2000 |
9d32cdd to
bea3690
Compare
| # needs to be ran wrapped in FHS for now | ||
| # because the default config refers to many FHS paths | ||
| ExecStart = "${cfg.package.fhs}/bin/onlyoffice-wrapper ${cfg.package.fileconverter}/bin/fileconverter"; | ||
| ExecStart = "${lib.getExe cfg.package.fhs} ${cfg.package.fileconverter}/bin/fileconverter"; |
There was a problem hiding this comment.
| ExecStart = "${lib.getExe cfg.package.fhs} ${cfg.package.fileconverter}/bin/fileconverter"; | |
| ExecStart = "${lib.getExe cfg.package.fhs} ${lib.getExe cfg.package.fileconverter}"; |
| }; | ||
| serviceConfig = { | ||
| ExecStart = "${cfg.package.fhs}/bin/onlyoffice-wrapper ${cfg.package.docservice}/bin/docservice"; | ||
| ExecStart = "${lib.getExe cfg.package.fhs} ${cfg.package.docservice}/bin/docservice"; |
There was a problem hiding this comment.
| ExecStart = "${lib.getExe cfg.package.fhs} ${cfg.package.docservice}/bin/docservice"; | |
| ExecStart = "${lib.getExe cfg.package.fhs} ${lib.getExe cfg.package.docservice}"; |
| --use-system=true | ||
|
|
||
| mkdir -p $out/bin | ||
| cp ${x2t}/bin/x2t $out/bin |
There was a problem hiding this comment.
| cp ${x2t}/bin/x2t $out/bin | |
| ln -s ${lib.getExe x2t} $out/bin |
There was a problem hiding this comment.
(see other comment - this would make sense but I've had to convert several ln -s to cp because the codebase uses calls that don't follow symlinks, so not obvious if this is possible here)
|
|
||
| mkdir -p $out/bin | ||
| cp ${x2t}/bin/x2t $out/bin | ||
| cat >$out/bin/DoctRenderer.config <<EOF |
There was a problem hiding this comment.
Why is a config file in $out/bin?
There was a problem hiding this comment.
Because x2t hard-codes looking for it there. Earlier I carried a patch in the onlyoffice-documentserver package to load it from ../etc instead, but that was a pain to maintain so I gave in and dropped that.
Euro-office promises to be more open to contributions, so maybe we can fix it upstream this time - but not sure if it should be a priority.
| echo Generating presentation themes | ||
| # creates temporary files next to sources... | ||
| mkdir working | ||
| cp ${x2t.components.sdkjs}/slide/themes/src/* working |
There was a problem hiding this comment.
| cp ${x2t.components.sdkjs}/slide/themes/src/* working | |
| ln -s ${x2t.components.sdkjs}/slide/themes/src/* working |
There was a problem hiding this comment.
this is worth testing, but generally a lot of the codebase is using calls that don't follow symlinks, so I had to convert a bunch of things that I initially used symlinks for to use cp.
| # equivalent of usr/bin/documentserver-flush-cache.sh, | ||
| # busts cache also when fonts collection changes | ||
| mkdir $out/var/www/onlyoffice/documentserver/web-apps | ||
| ${lndir}/bin/lndir -silent ${x2t.components.web-apps} $out/var/www/onlyoffice/documentserver/web-apps |
There was a problem hiding this comment.
| ${lndir}/bin/lndir -silent ${x2t.components.web-apps} $out/var/www/onlyoffice/documentserver/web-apps | |
| ${lib.getExe lndir} -silent ${x2t.components.web-apps} $out/var/www/onlyoffice/documentserver/web-apps |
There was a problem hiding this comment.
is this really a pattern we want to push even in 'simple' cases like this? do we have that written down in a style guide somewhere yet?
There was a problem hiding this comment.
is this really a pattern we want to push even in 'simple' cases like this?
Yes, it is IMHO. If someone creates an overlay that overrides lndir such that the main program is no longer called lndir, we still want it to work correctly as long as the mainProgram is set correctly.
There was a problem hiding this comment.
With lib.getExe we get a single source of truth for the relevant executable in a package. Yes, we want that.
There was a problem hiding this comment.
Doesn't that conflict with the advice to move from runCommand to mkDerivation and put the dependencies in nativeBuildInputs? Since people do that we can't expect overlays that change the main program to work anyway, right?
https://nixos.org/manual/nixpkgs/unstable/ also still uses the {pkg}/bin/pkg pattern in a bunch of places and doesn't document a clear preference.
There was a problem hiding this comment.
Doesn't that conflict with the advice to move from runCommand to mkDerivation and put the dependencies in nativeBuildInputs? Since people do that we can't expect overlays that change the main program to work anyway, right?
That's a fair point I hadn't thought about. I'll have to think on that.
https://nixos.org/manual/nixpkgs/unstable/ also still uses the {pkg}/bin/pkg pattern in a bunch of places and doesn't document a clear preference.
Instances of that pattern should 100% be replaced with lib.getExe or lib.getExe'.
There was a problem hiding this comment.
It seems that a .exe attribute would pay off in terms of UX, without performance cost:
The increase in allocation is offset by better collectability. Eval times appear shorter.
I think it'd be nice. Wdyt?
| ]; | ||
|
|
||
| extraBuildCommands = '' | ||
| mkdir -p $out/var/{lib/onlyoffice,www} |
|
For background: euro-office is a fork of onlyoffice, so a lot of the comments here are actually comments on the onlyoffice package (too). As onlyoffice-documentserver maintainer, I'm already in contact with onny to see if/how we can co-maintain to keep things reasonably in sync, stay tuned. |
| mkdir -p $out/web | ||
| mkdir -p $out/converter | ||
| mkdir -p $out/images | ||
| mkdir -p $out/fonts |
There was a problem hiding this comment.
Nitpick: this can be simplified
| mkdir -p $out/web | |
| mkdir -p $out/converter | |
| mkdir -p $out/images | |
| mkdir -p $out/fonts | |
| mkdir -p $out/{web,converter,images,fonts} |
Also, stdenvNoCC.mkDerivation instead of runCommand is my strong preference since:
- It has better compatibility with other tooling
- It lets you run
allfontsgenin the build phase and copy to$outin the install phase - It lets you add script dependencies such as
x2t,allfontsgen, etc... tonativeBuildInputs
|
|
||
| echo Generating fonts | ||
| export CUSTOM_FONTS_PATHS=${lib.concatStringsSep ":" extra-fonts} | ||
| ${x2t.components.allfontsgen}/bin/allfontsgen \ |
There was a problem hiding this comment.
Nit: either do the mkDerivation approach or
| ${x2t.components.allfontsgen}/bin/allfontsgen \ | |
| ${lib.getExe x2t.components.allfontsgen} \ |
| ${lib.getExe x2t.components.allthemesgen} \ | ||
| --converter-dir="$out/bin"\ | ||
| --src="working"\ | ||
| --output="$out/images" | ||
| ${x2t.components.allthemesgen}/bin/allthemesgen \ | ||
| --converter-dir="$out/bin"\ | ||
| --src="working"\ | ||
| --output="$out/images"\ | ||
| --postfix="ios"\ | ||
| --params="280,224" | ||
| ${x2t.components.allthemesgen}/bin/allthemesgen \ |
There was a problem hiding this comment.
| ${lib.getExe x2t.components.allthemesgen} \ | |
| --converter-dir="$out/bin"\ | |
| --src="working"\ | |
| --output="$out/images" | |
| ${x2t.components.allthemesgen}/bin/allthemesgen \ | |
| --converter-dir="$out/bin"\ | |
| --src="working"\ | |
| --output="$out/images"\ | |
| --postfix="ios"\ | |
| --params="280,224" | |
| ${x2t.components.allthemesgen}/bin/allthemesgen \ | |
| ${lib.getExe x2t.components.allthemesgen} \ | |
| --converter-dir="$out/bin"\ | |
| --src="working"\ | |
| --output="$out/images" | |
| ${lib.getExe x2t.components.allthemesgen} \ | |
| --converter-dir="$out/bin"\ | |
| --src="working"\ | |
| --output="$out/images"\ | |
| --postfix="ios"\ | |
| --params="280,224" | |
| ${lib.getExe x2t.components.allthemesgen} \ |
| echo "[]" > $out/var/www/onlyoffice/documentserver/sdkjs-plugins/plugin-list-default.json | ||
|
|
||
| mkdir -p $out/var/www/onlyoffice/documentserver/server/schema | ||
| cp -r ${server-src}/schema/* $out/var/www/onlyoffice/documentserver/server/schema |
There was a problem hiding this comment.
Nitpick: would you mind opening (and linking to in comments) upstream issues for not following symlinks? I'd do it myself except I'm not familiar with the codebase and don't have the time to test every instance of not following symlinks myself.
|
|
||
| ## required for bwrap --bind | ||
| chmod u+w $out/var | ||
| mkdir -p $out/var/lib/onlyoffice |
There was a problem hiding this comment.
It looks like this isn't used in the main derivation? Why is this here?
| common = buildNpmPackage (finalAttrs: { | ||
| name = "euro-office-server-Common"; | ||
| src = server-src; | ||
| sourceRoot = "${finalAttrs.src.name}/Common"; | ||
| npmDepsHash = "sha256-zFGqDtnNFzXCwp6uvK04GDMRG6BATv6ti3Wi8ikLjBU="; | ||
| dontNpmBuild = true; | ||
| postPatch = '' | ||
| # https://github.com/ONLYOFFICE/build_tools/blob/ef8153c053bed41909ceb0762b124f8fe7faa0a7/scripts/build_server.py#L34 | ||
| sed -e "s/^const buildVersion = '[0-9.]*'/const buildVersion = '${version}'/" -i sources/commondefines.js | ||
| ''; | ||
| postInstall = '' | ||
| ln -s $out/lib/node_modules/common $out/lib/node_modules/Common | ||
| ''; | ||
| }); | ||
| docservice = buildNpmPackage (finalAttrs: { | ||
| name = "euro-office-server-DocService"; | ||
| src = server-src; | ||
| sourceRoot = "${finalAttrs.src.name}/DocService"; | ||
| nativeBuildInputs = [ | ||
| pkg-config | ||
| ]; | ||
| buildInputs = [ | ||
| vips.dev | ||
| ]; | ||
| npmDepsHash = "sha256-eD7hyeIcSL0nLcmBE5+gDJcjT+LdUaqIZ+g5sPcn8HQ="; | ||
| npmFlags = [ "--loglevel=verbose" ]; | ||
| dontNpmBuild = true; | ||
| postInstall = '' | ||
| # it would be neater if this were a 'ln -s', but this is not possible | ||
| # because common/sources/notificationService.js has a circular dependency | ||
| # back on DocService | ||
| cp -r ${common}/lib/node_modules/common $out/lib/node_modules/Common | ||
| ln -s $out/lib/node_modules/coauthoring $out/lib/node_modules/DocService | ||
| ''; | ||
| }); | ||
| fileconverter = buildNpmPackage (finalAttrs: { | ||
| name = "euro-office-server-FileConverter"; | ||
| src = server-src; | ||
|
|
||
| sourceRoot = "${finalAttrs.src.name}/FileConverter"; | ||
|
|
||
| npmDepsHash = "sha256-zGLZBbQYV2z0HgQKISKVhclRKbMB8RYEX13H0mB6qJw="; | ||
|
|
||
| dontNpmBuild = true; | ||
|
|
||
| postInstall = '' | ||
| ln -s ${common}/lib/node_modules/common $out/lib/node_modules/Common | ||
| ln -s ${docservice}/lib/node_modules/coauthoring $out/lib/node_modules/DocService | ||
| ''; | ||
| }); |
There was a problem hiding this comment.
There's a good argument to be made for moving these into separate packages, or at least separate files. This is what we did for stalwart (originally separate files, subsequently separate packages).
|
Also: if |
| # it would be neater if this were a 'ln -s', but this is not possible | ||
| # because common/sources/notificationService.js has a circular dependency | ||
| # back on DocService | ||
| cp -r ${common}/lib/node_modules/common $out/lib/node_modules/Common | ||
| ln -s $out/lib/node_modules/coauthoring $out/lib/node_modules/DocService |
There was a problem hiding this comment.
Opened Euro-Office/server#17 which should resolve this issue and allow for proper symlinking
| postInstall = '' | ||
| ln -s ${common}/lib/node_modules/common $out/lib/node_modules/Common | ||
| ln -s ${docservice}/lib/node_modules/coauthoring $out/lib/node_modules/DocService | ||
| ''; |
There was a problem hiding this comment.
Euro-Office/server#17 should eliminate the need for this.
|
updated to 9.3.1 |
|
Hi, |
packaging euro-office-documentserver, a hard-fork of onlyoffice-documentserver and „an online office suite comprising viewers and editors“
run tests with:
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.