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

surli · 2024-10-30T12:57:05Z

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

+     * @return the hostname of the office server instance
+     * @since 16.10.0RC1
+     */
+    default String getServerHost()


Missing @Unstable annotation.

surli · 2024-10-30T12:58:50Z

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

+     * environment temporary directory)
+     * @see 16.10.0RC1
+     */
+    default String getWorkDir()


Also missing unstable annotation. And I think now we have an annotation when it's nullable, I need to double check that.

Now I would have considered that both null or empty would mean to fallback on default temp env.

surli · 2024-10-30T12:59:38Z

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

+    /**
+     * @see OfficeServerConfiguration#getServerHost()
+     */
+    private static final String DEFAULT_SERVER_HOST = "127.0.0.1";


Any reason to not expose the constant in OfficeServerConfiguration and reuse it here?

surli · 2024-10-30T13:01:50Z

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

        File workDir = this.environment.getTemporaryDirectory();
-        this.converter = new DefaultOfficeConverter(this.jodConverter, workDir);
+        if (StringUtils.isNotBlank(this.config.getWorkDir())) {


ok so it confirms it can be empty or null so the javadoc needs to be improved.

surli · 2024-10-30T13:02:42Z

...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]


It's 16.10.0RC1.
Btw you don't need that in other branches?

surli · 2024-10-30T13:03:21Z

...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.
+# openoffice.host = localhost


The default 127.0.0.1 should be used here no?

surli · 2024-10-30T13:03:31Z

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

+#-# [Since 16.10RC1]
+#-# Path to a folder where XWiki could exchange temporary files with the openoffice server.
+#-# If no path is provided, the temporary folder set for XWiki will be used.
+# openoffice.workDir = /tmp


Default should be empty no?

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).

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.

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

surli Oct 30, 2024

surli Oct 30, 2024

surli Oct 30, 2024

surli Oct 30, 2024

surli Oct 30, 2024

surli Oct 30, 2024

surli 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

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

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

Choose a reason for hiding this comment

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

vmassol Oct 30, 2024 •

edited

Loading

gdelhumeau commented Nov 12, 2024 •

edited

Loading