-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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 Collabora Online #330708
Add Collabora Online #330708
Conversation
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.
We cannot use freeform settings here?
python3 | ||
python3.pkgs.lxml | ||
python3.pkgs.polib | ||
rsync |
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.
We should probably build a python env with the modules
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.
Why? This pattern is widely used in nixpkgs: https://github.com/search?q=repo%3ANixOS%2Fnixpkgs+"++python3.pkgs."&type=code
These only used during the build, not in runtime.
postPatch = | ||
'' | ||
cp ${./package-lock.json} ${finalAttrs.npmRoot}/package-lock.json | ||
patchShebangs . |
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.
Please be a bit more specific than everything. Also we normally place postPatch right after patches and src
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.
Updated.
postInstall = '' | ||
cp etc/ca-chain.cert.pem etc/cert.pem etc/key.pem $out/etc/coolwsd | ||
''; |
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.
Don't we want to take ca-certs?
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.
WDYM? These self-signed certificates are provided for testing purposes.
In NixOS module, they're enabled by default unless overridden by the user.
mount_namespaces = lib.mkOption { | ||
type = lib.types.bool; | ||
default = false; | ||
description = ''Use mount namespaces instead of coolmount.''; | ||
}; |
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.
Why not use them?
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.
Enabled by default in the updated commit.
About configuration: can we just keep the defaults for most stuff, then edit whatever overrides in? And probably it would be done NixOS/rfcs#42 style with only a few options defined explicitly but whatever the user puts deep into the structure being used for overrides, too… |
Also silly question: is it a good idea to add the package first and discuss the configuration generation in a follow-up? |
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.
The package diff LGTM apart from a few smaller things.
The module though... It's a beast. I don't think it's reasonable to accept such a huge module.
- Would it be possible to only pass a subset of options and have the application use the upstream default values on its own?
- If that is possible, would it be possible to make this a RFC42-style
settings
option so that we don't have to declare any options ourselves? - In the docker image, the app takes a few env vars to configure the most important things. Perhaps we could limit the module to those (via RFC42-style env var option) and then give the user the ability to pass a plain config file text. This would significantly slim the module while still providing useful high-level options to the user.
# Use setcap'd wrappers from the wrappers dir, not from the "application path". | ||
+ '' | ||
substituteInPlace common/JailUtil.cpp --replace-fail \ | ||
'Poco::Path(Util::getApplicationPath(), "coolmount").toString()' \ | ||
'std::string("/run/wrappers/bin/coolmount")' | ||
substituteInPlace wsd/COOLWSD.cpp --replace-fail \ | ||
' forKitPath += "coolforkit";' \ | ||
' forKitPath = "/run/wrappers/bin/coolforkit";' | ||
'' | ||
# In the nix build, COOLWSD_VERSION_HASH becomes the same as COOLWSD_VERSION, e.g. `24.04.3.5`. | ||
# The web server that serves files from `/browser/$COOLWSD_VERSION_HASH`, doesn't expect the | ||
# hash to contain dots. | ||
+ '' | ||
substituteInPlace wsd/FileServer.cpp --replace-fail \ | ||
'Poco::RegularExpression gitHashRe("/([0-9a-f]+)/");' \ | ||
'Poco::RegularExpression gitHashRe("/([0-9a-f.]+)/");' \ | ||
''; |
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.
These should probably be patches.
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.
Honestly, at that length of the pattern (spurious hits are not too likely) I'd say either way is fine (LibreOffice packaging uses a mix, because long substitutions are easier to maintain than patches)
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.
Removed the first change as it's not required if mount_namespaces
enabled. (enabled in the NixOS module by default)
Moved the second change to ./fix-file-server-regex.patch
.
33ce05d
to
91a524a
Compare
Update:
I've considered the following alternative freeform approaches:
|
This should probably be reported upstream, even if you already implemented a workaround. |
<host>::1</host> | ||
</post_allow></net> | ||
``` | ||
in `coolwsd.xml`, or `--o:net.post_allow.host[0]='127\.0\.0\.1 --o:net.post_allow.host=::1` in |
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.
I'm assuming this should be:
in `coolwsd.xml`, or `--o:net.post_allow.host[0]='127\.0\.0\.1 --o:net.post_allow.host=::1` in | |
in `coolwsd.xml`, or `--o:net.post_allow.host[0]='127\.0\.0\.1 --o:net.post_allow.host[1]=::1` in |
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.
Fixed.
options.services.collabora-online = { | ||
enable = lib.mkEnableOption "collabora-online"; | ||
|
||
package = lib.mkPackageOption pkgs "collabora-online" { }; |
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.
package = lib.mkPackageOption pkgs "collabora-online" { }; | |
package = lib.mkPackageOption pkgs "Collaborate Online" { default = "collabora-online"; }; |
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.
Fixed.
(!!) yes, please report this, thanks! |
Reported upsteam: CollaboraOnline/online#10049. |
Description of changes
URL: https://www.collaboraonline.com
This PR adds Collabora Online Development Edition (CODE). CODE is a way to run LibreOffice in the browser. Continuation of #329525.
In this PR two things are introduced:
collabora-online
.cc: @7c6f434c
fixes #333457
How to run
This guide will let you run a NixOS VM with Collabora Online and Nextcloud. The purpose is to provide a quick start and to demonstrate that it works, so it's messy and insecure.
Note: I am still yet to figure out how to properly run host-accessible NixOS VMs1, and not port-forwarded ones. That's why the hostname of the host and the VM should match.
Build VM
Grab this definition of NixOS VM and replace
myawesomehostname
with the hostname of your host. (the VM and the host should have the same hostname)vm.nix
Build it with the following command:
Run VM
Sudo is required to bind the VM to port 80.
Login
In a browser, go to
http://myawesomehostname
.It will prompt you with a login page. Enter
root
anda
.Create a new document
In the top left corner select "Files", then "+ New", then "New Document". Press "Create".
Click on the newly created document to open it.
The Collabora Online editor should appear.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.
Footnotes
They should use qemu's
-net tap
, I guess. ↩