XWIKI-22481: Add the ability to use a LibreOffice binary from another server #3610

gdelhumeau · 2024-10-30T11:45:48Z

Jira URL

https://jira.xwiki.org/browse/XWIKI-22481

Changes

Description

I have introduced 2 new properties in xwiki.properties:

openoffice.host to set the hostname of the openoffice server (in case it not reachable at "localhost")
openoffice.workDir to set the folder where the files are exchanged, temporary, between XWiki and the openoffice server (same: useful in a kubernetes environment where we need to mount a common volume between the 2 containers).

Clarifications

I have not made commit on xwiki-platform for a while, so I prefer asking for a quick confirmation before merging. Thank you!

I have not built on the current master branch, but it built successfully against the 16.4.4 tag.

… server.

… server. * Fix checkstyle issue.

gdelhumeau · 2024-10-30T11:46:54Z

...office-importer/src/main/java/org/xwiki/officeimporter/server/OfficeServerConfiguration.java

+     */
+    default String getWorkDir()
+    {
+        return null;


Not ideal, but I did not want to break the stable interface. The failover is implemented by the component that call getWorkDir().

You might want to use Optional<String> as the return value, see https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/#HOptional

vmassol · 2024-10-30T12:58:26Z

...orm-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm

@@ -350,6 +350,10 @@ rendering.transformations = $xwikiRenderingTransformations
 #-# 1 - Externally managed (local) server instance.
 # openoffice.serverType = 0

+#-# [Since 16.10RC1]
+#-# Hostname used for connecting to the openoffice server instance.


Missing the explanation about what is the default value if not specified.

vmassol · 2024-10-30T12:58:56Z

...orm-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm

@@ -371,6 +375,11 @@ rendering.transformations = $xwikiRenderingTransformations
 #-# If no path is provided, a default value will be calculated based on the operating environment.
 # openoffice.profilePath = /home/user/.openoffice.org/3

+#-# [Since 16.10RC1]
+#-# Path to a folder where XWiki could exchange temporary files with the openoffice server.


s/could/will ?

vmassol · 2024-10-30T13:00:09Z

...ice-importer/src/main/java/org/xwiki/officeimporter/internal/server/DefaultOfficeServer.java

@@ -137,6 +138,7 @@ public void initialize() throws OfficeServerException
            ExternalOfficeManager.Builder externalProcessOfficeManager = ExternalOfficeManager.builder();
            externalProcessOfficeManager.portNumbers(this.config.getServerPorts());
            externalProcessOfficeManager.connectOnStart(true);
+            externalProcessOfficeManager.hostName(config.getServerHost());


Are we sure that 127.0.0.1 is the default? (vs localhost for example).

Another option is to set the a host only if not empty in the XWiki config.

I confirm that 127.0.0.1 is the default -> https://github.com/jodconverter/jodconverter/blob/master/jodconverter-local/src/main/java/org/jodconverter/local/office/ExternalOfficeManager.java#L60

...office-importer/src/main/java/org/xwiki/officeimporter/server/OfficeServerConfiguration.java

...src/main/java/org/xwiki/officeimporter/internal/server/DefaultOfficeServerConfiguration.java

...ice-importer/src/main/java/org/xwiki/officeimporter/internal/server/DefaultOfficeServer.java

...orm-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm

gdelhumeau · 2024-10-30T14:26:16Z

Thank you for all your comments! It was definitively needed!

… server. * Use Optional<String> instead of plain String in OfficeServerConfiguration#getWorkDir(). * Improve the comments in xwiki.properties.

gdelhumeau · 2024-10-30T15:00:11Z

I have pushed my changes, thanks!

vmassol · 2024-10-30T15:04:31Z

@gdelhumeau I don't think I've seen any update to the Admin UI at http://localhost:8080/xwiki/bin/admin/XWiki/XWikiPreferences?editor=globaladmin&section=XWiki.OfficeImporterAdmin

Isn't that required? For example to display the server host used.

Thanks

vmassol · 2024-10-30T15:06:46Z

Also we already have this config:

#-# [Since 1.9M2]
#-# Type of the openoffice server instance used by officeimporter component.
#-# 0 - Internally managed server instance. (Default)
#-# 1 - Externally managed (local) server instance.
# openoffice.serverType = 0

I'm not sure how it interacts with your change. On one hand, a value of 1 is what you're doing since it's externally managed, but OTOH it's not a local server instance.

Does it mean that right now we can use your new config property AND keep openoffice.serverType = 0? Sounds weird, no?

Thx

gdelhumeau · 2024-10-30T16:40:07Z

These changes only apply when openoffice.serverType = 1. I think I should mention it in the xwiki.properties file (and also mention that 1 doesn't imply it is local).

Actually we have a constant SERVER_TYPE_EXTERNAL_REMOTE (with the value 2). This is actually what I am doing.

… server. * Handle the SERVER_TYPE_EXTERNAL_REMOTE use-case. * Update OfficeImporterAdmin.

gdelhumeau · 2024-10-30T18:05:46Z

I have updated the code and the documentation to handle SERVER_TYPE_EXTERNAL_REMOTE.
I suppose the proper implementation would not need to have a shared work directory, but it's already an improvement.

I have also updated the UI.

gdelhumeau · 2024-11-12T17:43:05Z

@vmassol @surli Did you have the chance to take a look? Do you want a proper proposal on the forum? Maybe you were expecting a proper "remote" behavior where we wouldn't have to share any drive but I don't have the time to implement this and I think what I'm proposing is already an improvement (at least it fixes the problem I have with my Docker/Kubernetes use-case).

vmassol · 2024-11-13T10:39:03Z

...office-importer/src/main/java/org/xwiki/officeimporter/server/OfficeServerConfiguration.java

+     * Default host name of the office server instance.
+     * Coming from org.jodconverter.local.office.ExternalOfficeManager.DEFAULT_HOSTNAME
+     * @since 16.10.0RC1
+     * @since 16.4.5


should be 16.4.6 (as used below).

And that's also Unstable

vmassol · 2024-11-13T10:41:11Z

...ki-platform-office/xwiki-platform-office-ui/src/main/resources/XWiki/OfficeImporterAdmin.xml

+          &lt;dt&gt;&lt;label&gt;$escapetool.html($services.localization.render('xe.officeimporter.openoffice.workdir'))&lt;/label&gt;&lt;/dt&gt;
+          &lt;dd&gt;$escapetool.html($services.officemanager.config.workDir.get())&lt;/dd&gt;
+        #end
+        ###


I'm wondering if it's useful to always display that information. Looks a bit technical and more for debug purposes. Let's leave it like this for now but we may want to only display it for advanced users or when in debug mode (debug = true in the query string for example).

vmassol · 2024-11-13T10:41:42Z

...ki-platform-office/xwiki-platform-office-ui/src/main/resources/XWiki/OfficeImporterAdmin.xml

+        ###
+        #if ($services.officemanager.config.serverType == 2)
+          &lt;dt&gt;
+            &lt;label&gt;$escapetool.html($services.localization.render('xe.officeimporter.openoffice.serverhost'))&lt;/label&gt;


hmm this is using old translation key format, I think we need to fix this. See https://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices#HTranslationPropertyNaming

vmassol · 2024-11-13T10:42:06Z

...ki-platform-office/xwiki-platform-office-ui/src/main/resources/XWiki/OfficeImporterAdmin.xml

+        ### Work directory
+        ###
+        #if ($services.officemanager.config.workDir.isPresent())
+          &lt;dt&gt;&lt;label&gt;$escapetool.html($services.localization.render('xe.officeimporter.openoffice.workdir'))&lt;/label&gt;&lt;/dt&gt;


same comment re translation key format

vmassol · 2024-11-13T10:44:00Z

...orm-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm

@@ -348,8 +348,15 @@ rendering.transformations = $xwikiRenderingTransformations
 #-# Type of the openoffice server instance used by officeimporter component.
 #-# 0 - Internally managed server instance. (Default)
 #-# 1 - Externally managed (local) server instance.
+#-# 2 - Externally managed (remotely) server instance. (Since 16.10.0RC1, 16.4.6)


s/remotely/remote

vmassol · 2024-11-13T10:45:06Z

...orm-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm

+#-# [Since 16.4.6]
+#-# Path to a folder where XWiki will exchange temporary files with the openoffice server.
+#-# If no path is provided, the temporary folder set for XWiki will be used.
+#-# If openoffice.serverType = 2, this folder should be a shared volume between the two servers.


should or must?

vmassol · 2024-11-13T10:46:35Z

@vmassol @surli Did you have the chance to take a look? Do you want a proper proposal on the forum? Maybe you were expecting a proper "remote" behavior where we wouldn't have to share any drive but I don't have the time to implement this and I think what I'm proposing is already an improvement (at least it fixes the problem I have with my Docker/Kubernetes use-case).

Thanks @gdelhumeau . The PR looks good to me, just some small details I've commented about. @surli ok for you?

surli · 2024-11-13T11:00:00Z

@surli ok for you?

yes sounds good to me

gdelhumeau added 2 commits October 30, 2024 12:30

XWIKI-22481: Add the ability to use a LibreOffice binary from another…

be713cd

… server.

XWIKI-22481: Add the ability to use a LibreOffice binary from another…

5319d2a

… server. * Fix checkstyle issue.

gdelhumeau commented Oct 30, 2024

View reviewed changes

surli self-assigned this Oct 30, 2024

vmassol reviewed Oct 30, 2024

View reviewed changes

surli requested changes Oct 30, 2024

View reviewed changes

XWIKI-22481: Add the ability to use a LibreOffice binary from another…

22c9688

… server. * Use Optional<String> instead of plain String in OfficeServerConfiguration#getWorkDir(). * Improve the comments in xwiki.properties.

XWIKI-22481: Add the ability to use a LibreOffice binary from another…

2ad3375

… server. * Handle the SERVER_TYPE_EXTERNAL_REMOTE use-case. * Update OfficeImporterAdmin.

vmassol reviewed Nov 13, 2024

View reviewed changes

vmassol self-assigned this Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XWIKI-22481: Add the ability to use a LibreOffice binary from another server #3610

XWIKI-22481: Add the ability to use a LibreOffice binary from another server #3610

gdelhumeau commented Oct 30, 2024

gdelhumeau Oct 30, 2024

vmassol Oct 30, 2024

vmassol Oct 30, 2024

vmassol Oct 30, 2024

vmassol Oct 30, 2024 •

edited

Loading

vmassol Oct 30, 2024

gdelhumeau commented Oct 30, 2024

gdelhumeau commented Oct 30, 2024

vmassol commented Oct 30, 2024

vmassol commented Oct 30, 2024

gdelhumeau commented Oct 30, 2024

gdelhumeau commented Oct 30, 2024

gdelhumeau commented Nov 12, 2024 •

edited

Loading

vmassol Nov 13, 2024

surli Nov 13, 2024

vmassol Nov 13, 2024

vmassol Nov 13, 2024 •

edited

Loading

vmassol Nov 13, 2024

vmassol Nov 13, 2024

vmassol Nov 13, 2024

vmassol commented Nov 13, 2024 •

edited

Loading

surli commented Nov 13, 2024

XWIKI-22481: Add the ability to use a LibreOffice binary from another server #3610

Are you sure you want to change the base?

XWIKI-22481: Add the ability to use a LibreOffice binary from another server #3610

Conversation

gdelhumeau commented Oct 30, 2024

Jira URL

Changes

Description

Clarifications

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmassol Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdelhumeau commented Oct 30, 2024

gdelhumeau commented Oct 30, 2024

vmassol commented Oct 30, 2024

vmassol commented Oct 30, 2024

gdelhumeau commented Oct 30, 2024

gdelhumeau commented Oct 30, 2024

gdelhumeau commented Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmassol Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmassol commented Nov 13, 2024 • edited Loading

surli commented Nov 13, 2024

vmassol Oct 30, 2024 •

edited

Loading

gdelhumeau commented Nov 12, 2024 •

edited

Loading

vmassol Nov 13, 2024 •

edited

Loading

vmassol commented Nov 13, 2024 •

edited

Loading