Modifications to get DocumentServer@core compiling and working under FreeBSD #297

Teetoow · 2020-08-25T11:52:08Z

Here is a patch in order to get the core package of DocumentServer compiling and working under FreeBSD.
Most of the patch is adding "FreeBSD" to the list of "Linux" preproc conditions.
2-3 lines of code (mainly errors or cast problem) have been updated due to errors with clang compiler (the one used under FreeBSD). It does not changes something for other platforms.
Last the Linux code used to get the common working directory is not working under FreeBSD. Thus a specific one has been added.

Explanations: OnlyOffice depends on v8 which needs specific compilation tools under FreeBSD (native v8 compilation tools are not written for FreeBSD. Thus under FreeBSD we use node10 available in the FreeBSD port compiled with the "shared" option. In that way libnode.so is used in order to get the v8 code). Since v8 library is got from FreeBSD ports, we also want to use the native ICU library, boost library, openssl library etc. Moreover it lets the sysadmin getting the latest security patches for these libraries and avoid security holes (without recompiling onlyoffice).

Correct an error in the code which leads to an error during compilation with clang.

Correct a syntax error in the code which leads to an error during compilation with clang. Add _gcvt function which does not exists under FreeBSD.

- Correct syntax error in the code which leads to error when compiling with clang (not casting signed 8-bits values greater than 0x80). - Add a specific code which finds the common working directory (using /proc for that purpose under FreeBSD is not possible)

Correct syntax error in the code which leads to an error with clang compiler: returning NULL (void*) to a variable which is unsigned int

MikaelUrankar · 2020-08-25T12:00:26Z

ASCOfficeDocFile/DocDocxConverter/StringTable.h

@@ -77,13 +77,13 @@ namespace DocFileFormat
 		}

 		StringTable( VirtualStreamReader *reader, int code_page_ ): 
-                            code_page(code_page_), fExtend(false), cbData(0), cbExtra(0), DataExtra(NULL)
+                            code_page(code_page_), fExtend(false), cbData(0), cbExtra(0)


DataExtra(0)

I don't understand the usefulness of this line since DataExtra is a std::vector which is well initialized without any specific constructor call.

I asked dev team and we think better variant is to define #define NULL 0 in #if
(not sure if translated that right, not good in C Programming)

I would be very interested in having a good explaination about: what is the aim of this line and why using NULL rather than an integer?
Following the vector documentation constructors are:

explicit vector (const allocator_type& alloc = allocator_type()); explicit vector (size_type n, const value_type& val = value_type(), const allocator_type& alloc = allocator_type()); template <class InputIterator> vector (InputIterator first, InputIterator last,const allocator_type& alloc = allocator_type()); vector (const vector& x);

Since NULL might be assimilated by some compilers to 0 it seems this line is equivalent to DataExtra(0).
First, using DataExtra(0) or DataExtra() is equivalent following the documentation (default constructor: Constructs an empty container, with no elements.).
Next, in C NULL refers to a pointer. That's why I don't understand why using it in that precise case?
Or maybe I missed something but from my point of view this explicit constructor call is just useless (and syntaxically wrong).

Last but not least redefining NULL to 0 might not be a good idea since cast checks would be wrong

I asked our dev @ElenaSubbotina about this part and this may be some leftover from early days

We need some time to figure out if this code really needed in that form and could changes break something else, but currently we have not enought resource, since we are planning to release next major update soon and all work in there

I hope we'll have time to look into it again after major release

Thank you for you job

ok I understand. I hope you'll have time soon. I also hope most of the update (in particular all the preproc conditions and specific code surrounded by FreeBSD preproc conditions) would be added in the next major release.
Maybe you can surround this declaration with #ifdef (and all the code updates you didn't checked yet) in order to have the next major release compiling under FreeBSD (from my point of view, I'm not asking you to try to compile each release under FreeBSD but at least keep patches for that purpose in the master repository).
As you may see some people want/wait/need that feature.

I also hope most of the update (in particular all the preproc conditions and specific code surrounded by FreeBSD preproc conditions) would be added in the next major release.

Sorry, I don't think we can do that in next release
I'm sure that in 99% of those statements are correct and will good, but core is our base project and any major redone will require a lot of testing from QA and we do not have time for this in our release schedule

even with preproc conditions ?

even with preproc conditions ?

Sorry, no
New release in final stage of testing and we are not 100% this changes will not cause any side-effects and we miss our deadlines if there will be sideeffects

ok. So I hope it will be added as soon as possible.

MikaelUrankar · 2020-08-25T12:00:34Z

ASCOfficeDocFile/DocDocxConverter/StringTable.h

 		{
            parse( reader, (unsigned int)reader->GetPosition(), 0, false );
 		}

 		StringTable( POLE::Stream* tableStream, unsigned int fc, unsigned int lcb, int nWordVersion, bool bReadExta = false) :
-												code_page(1250), fExtend(false), cbData(0), cbExtra(0), DataExtra(NULL)
+												code_page(1250), fExtend(false), cbData(0), cbExtra(0)


DataExtra(0)

I don't understand the usefulness of this line since DataExtra is a std::vector which is well initialized without any specific constructor call.

Common/DocxFormat/Source/DocxFormat/Docx.cpp

MikaelUrankar · 2020-08-25T12:03:16Z

DesktopEditor/common/File.cpp

+
+        memset(buf, 0, NS_FILE_MAX_PATH);
+        if (sysctl(mib, 4, &buf, &size, NULL, 0) != 0) {
+            size = readlink("/proc/curproc/file", buf, size - 1);


/proc is not mounted by default, not sure it's worth keeping it.

You can have /proc mounted. sysctl might be enough for most of the cases but if not (I don't know in which case it wouldn't work..) you could still mount /proc and keep that code working

As you want, I never saw a sysctl failing and if it's the case you're probably doomed somehow.

DesktopEditor/fontengine/ApplicationFonts.cpp

DesktopEditor/raster/JBig2/source/LeptonLib/readfile.cpp

MikaelUrankar · 2020-08-25T12:08:47Z

Makefile

@@ -40,6 +44,18 @@ else
 		PACKAGE_VERSION := $(PRODUCT_VERSION)-$(BUILD_NUMBER)
 		ARCH_REPO_DIR := linux
 	endif
+	ifeq ($(UNAME_S),FreeBSD)
+		PLATFORM := linux


Why not PLATFORM := freebsd ?

it would be great to use "PLATFORM := freebsd" but all the compilation files would need to be changed without any improvments on the results. Since it changes nothing i prefered keep it like that. It needs few changes and everything is fine!

which files? It seems to only be used in the Makefile and seems useless.

you're right: in particular in Makefiles and QMake files too.
For the moment I prefer minimizing updates. When Makefiles will be removed we (you?) will be able to do more proper things.

MikaelUrankar · 2020-08-25T12:12:55Z

Common/DocxFormat/Source/XlsxFormat/Common.cpp

@@ -48,6 +48,11 @@ char* gcvt(double x, int n, char* b)
 #define _gcvt gcvt
 #endif

+#ifdef __FreeBSD__
+#define _gcvt(x,n,b) sprintf(b, "%.17g", x)


Android uses:

char* gcvt(double x, int n, char* b) { sprintf(b, "%.*g", n, x); return b; } X2

see XtConverter/build/Android/libx2t/src/main/cpp/workaround/gcvt/gcvt.c

it's nearly the same result..

MikaelUrankar · 2020-08-25T12:13:11Z

Common/DocxFormat/Source/XlsxFormat/Common.cpp

+#ifdef __FreeBSD__
+#define _gcvt(x,n,b) sprintf(b, "%.17g", x)
+#endif
+


what do you mean?

what do you mean?

extra blank line

there's more than a blank line.. I hope it's not the most important thing of this patch..
Should we really care about that?

ShockwaveNN · 2020-08-25T13:22:22Z

Well, thats a lot of changes, I'm not sure that dev team will have a lot of time to review it

DesktopEditor/raster/JBig2/source/LeptonLib/readfile.cpp

ShockwaveNN · 2020-08-25T13:30:11Z

ASCOfficeDocFile/DocDocxConverter/StringTable.h

@@ -77,13 +77,13 @@ namespace DocFileFormat
 		}

 		StringTable( VirtualStreamReader *reader, int code_page_ ): 
-                            code_page(code_page_), fExtend(false), cbData(0), cbExtra(0), DataExtra(NULL)
+                            code_page(code_page_), fExtend(false), cbData(0), cbExtra(0)


I asked dev team and we think better variant is to define #define NULL 0 in #if
(not sure if translated that right, not good in C Programming)

ShockwaveNN · 2020-08-25T13:34:00Z

We are planning to backport your changes in our code manually, with careful testing, since we fear some of this changes can broke some other OS variants (please do not forget, that this code not only used on Linux, but also on Mac, iOS and Android)

sankayop · 2021-01-26T07:56:43Z

Hi @ShockwaveNN
do you have any idea when this pullrequest will be added?
While implementing it manually to the current master version of oO (core, sdkjs, web-apps, ...), I found out you applied it partially but not completely?!

BSD users will be greatfull :)

ShockwaveNN · 2021-01-26T07:58:23Z

@sankayop Sorry, I cannot promise anything, since I'm only a QA, but our dev team promise me this:#297 (comment)

sankayop · 2021-01-26T08:00:38Z

@sankayop Sorry, I cannot promise anything, since I'm only a QA, but our dev team promise me this:#297 (comment)

Thanks, still struggling to get it right so that's why I'm interested :)

ShockwaveNN · 2021-01-26T08:06:08Z

Thanks, still struggling to get it right so that's why I'm interested :)

As I understand some of this changes may broke compatibility with some prehistoric OSes like WinXP which we support (some of our paid clients using it) and may broke Android\iOS

But this is how it's told to me, I'm not good at cpp code

Teetoow · 2021-02-28T14:25:32Z

Since there is preprocessor conditions everywhere for FreeBSD code it won't break something ;)

CLAassistant · 2021-03-22T08:38:06Z

All committers have signed the CLA.

Teetoow added 8 commits August 25, 2020 13:42

Add FreeBSD to the preproc conditions where Linux is also set.

521570b

Correct an error in the code which leads to an error during compilation with clang.

Add FreeBSD to the preproc conditions where Linux is also set.

2ea350d

Correct a syntax error in the code which leads to an error during compilation with clang. Add _gcvt function which does not exists under FreeBSD.

Add support of FreeBSD platform with clang (native) compiler.

31e4d7f

Add FreeBSD to the preproc conditions where Linux is also set.

5f57938

Add FreeBSD to the preproc conditions where Linux is also set.

3219051

Correct syntax error in the code which leads to an error with clang compiler: returning NULL (void*) to a variable which is unsigned int

Add FreeBSD compilation support

f584744

MikaelUrankar reviewed Aug 25, 2020

View reviewed changes

ShockwaveNN suggested changes Aug 25, 2020

View reviewed changes

Teetoow added 2 commits August 26, 2020 09:13

Under FreeBSD fonts are stores into /usr/local/share/fonts

8d8dd40

Error during a commit which removed casts. Reverting the commit

0580d0a

Teetoow requested a review from ShockwaveNN August 26, 2020 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifications to get DocumentServer@core compiling and working under FreeBSD #297

Modifications to get DocumentServer@core compiling and working under FreeBSD #297

Teetoow commented Aug 25, 2020

MikaelUrankar Aug 25, 2020

Teetoow Aug 25, 2020

ShockwaveNN Aug 25, 2020

Teetoow Aug 26, 2020 •

edited

Loading

ShockwaveNN Aug 26, 2020

Teetoow Aug 26, 2020 •

edited

Loading

ShockwaveNN Aug 26, 2020

Teetoow Aug 26, 2020

ShockwaveNN Aug 26, 2020

Teetoow Aug 26, 2020

MikaelUrankar Aug 25, 2020

Teetoow Aug 25, 2020

MikaelUrankar Aug 25, 2020

Teetoow Aug 25, 2020

MikaelUrankar Aug 25, 2020

MikaelUrankar Aug 25, 2020

Teetoow Aug 25, 2020

MikaelUrankar Aug 25, 2020

Teetoow Aug 26, 2020

MikaelUrankar Aug 25, 2020

Teetoow Aug 25, 2020

MikaelUrankar Aug 25, 2020

Teetoow Aug 25, 2020

MikaelUrankar Aug 26, 2020

Teetoow Aug 26, 2020

ShockwaveNN commented Aug 25, 2020

ShockwaveNN Aug 25, 2020

ShockwaveNN commented Aug 25, 2020

sankayop commented Jan 26, 2021

ShockwaveNN commented Jan 26, 2021

sankayop commented Jan 26, 2021

ShockwaveNN commented Jan 26, 2021

Teetoow commented Feb 28, 2021 •

edited

Loading

CLAassistant commented Mar 22, 2021 •

edited

Loading

Modifications to get DocumentServer@core compiling and working under FreeBSD #297

Are you sure you want to change the base?

Modifications to get DocumentServer@core compiling and working under FreeBSD #297

Conversation

Teetoow commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Teetoow Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Teetoow Aug 26, 2020 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShockwaveNN commented Aug 25, 2020

Choose a reason for hiding this comment

ShockwaveNN commented Aug 25, 2020

sankayop commented Jan 26, 2021

ShockwaveNN commented Jan 26, 2021

sankayop commented Jan 26, 2021

ShockwaveNN commented Jan 26, 2021

Teetoow commented Feb 28, 2021 • edited Loading

CLAassistant commented Mar 22, 2021 • edited Loading

Teetoow Aug 26, 2020 •

edited

Loading

Teetoow Aug 26, 2020 •

edited

Loading

Teetoow commented Feb 28, 2021 •

edited

Loading

CLAassistant commented Mar 22, 2021 •

edited

Loading