Add support for MinGW #8321

raedrizqie · 2024-11-16T05:18:39Z

No description provided.

hvlad · 2024-11-16T18:21:43Z

With all respect to the job done:

why you need MinGW build for Firebird ?
who else need it ?
who will support it ?

raedrizqie · 2024-11-16T18:30:55Z

Hi @hvlad
Firebird was already packaged on MSYS2 environments here: https://packages.msys2.org/base/mingw-w64-firebird since v5.0.0
It is a dependency of Qt5, Qt6 and SOCI.
I am not asking upstream to provides binaries for MinGW, but it will be easier for us on MSYS2 with less patching downstream.

Thanks for your considerations..

AlexPeshkoff · 2024-11-18T08:43:37Z

@hvlad We have a lot of old dead ports in the tree. What a problem if we have one currently working? We are not enforced to advertise it's presence. It will be alive as long as MSYS2 wants to support it.
I suggest to look closer at this PR. There are some issues with particular suggested changes - but in general it can be accepted on my mind.

hvlad · 2024-11-18T10:22:34Z

I will not object if it:

will be supported - there is no promises and no answer on my question so far,
will not interfere main Windows build.

Also, I have no MSYS2 env to check this changes and have no plans to install it.
I can't review changes in build system, as for the code - changes like in version.rc, protocol.cpp, isc.cpp looks weird for me.

AlexPeshkoff · 2024-11-18T10:37:38Z

Can't say for version.rc (STRINGIZE is typical source of problems), but protocol.cpp (and other #if => #ifdef changes) and specially isc.cpp look weird.

But before going to details more generic question to be answered - accept ports or not? Certainly, new ports should not break existing nad actively used one. What about support - I doubt anyone can give you strong guarantees :)

hvlad · 2024-11-18T10:57:24Z

more generic question to be answered - accept ports or not?

Do we need more ports - yes, why not.
Do we need unsupported ports - I doubt it.

Note, we'll have massive changes in master soon...

raedrizqie · 2024-11-18T11:36:33Z

I will not object if it:

will be supported - there is no promises and no answer on my question so far,

Currently, I am the maintainer for this package on MSYS2. So, I will give support for this MinGW port.

will not interfere main Windows build.

Sure, I will try not to mess with MSVC build.

Also, I have no MSYS2 env to check this changes and have no plans to install it. I can't review changes in build system, as for the code - changes like in version.rc, protocol.cpp, isc.cpp looks weird for me.

I will share the link of the successfull build on MSYS2, later.

For version.rc, I will revert the STRINGIZE patch
For inet.cpp, protocol.cpp and remote.cpp, GCC will error out because COMPRESS_DEBUG is not defined.
For isc.cpp, I will revert it.. (it is just to silence some warnings)

aafemt · 2024-11-18T11:40:39Z

For inet.cpp, protocol.cpp and remote.cpp, GCC will error out because COMPRESS_DEBUG is not defined.

For custom GCC options there is prefix.mingw where you can disable warning about usage of undefined macros.

hvlad · 2024-11-18T12:08:51Z

For inet.cpp, protocol.cpp and remote.cpp, GCC will error out because COMPRESS_DEBUG is not defined.

How it builds on Linux then ?

raedrizqie · 2024-11-18T12:14:22Z

Maybe Linux adds -DCOMPRESS_DEBUG on its CXXFLAGS? I didn't check..

aafemt · 2024-11-18T12:15:49Z

Also, I have no MSYS2 env to check this changes and have no plans to install it.

MSYS2 is installed in GitHub workers. It should be possible to add this build to workflows.

hvlad · 2024-11-18T12:17:55Z

Maybe Linux adds -DCOMPRESS_DEBUG on its CXXFLAGS? I didn't check..

I'm sure - no. This is for DEBUG, as it name implies ;)

raedrizqie · 2024-11-18T12:22:26Z

Maybe Linux adds -DCOMPRESS_DEBUG on its CXXFLAGS? I didn't check..

I'm sure - no. This is for DEBUG, as it name implies ;)

I just add -DCOMPRESS_DEBUG=0 for MinGW build

aafemt · 2024-11-18T12:23:08Z

Maybe Linux adds -DCOMPRESS_DEBUG on its CXXFLAGS? I didn't check..

It is contrary: prefix.mingw adds -Wundef to compilation command-line while Linux builds don't.

aafemt · 2024-11-18T12:25:32Z

builds/posix/Makefile.in.plugins_examples


 all: udrcpp_example dc_example kh_example crypt_app

+ifeq ($(PLATFORM),win32)


If you need platform-depended modifications to makefiles - do them in prefix file, not here.

Ok.. I will change it

aafemt · 2024-11-18T12:29:11Z

I just add -DCOMPRESS_DEBUG=0 for MinGW build

No, it would be a wrong fix. Checks in files must be changed to #if defined(COMPRESS_BUILD) && COMPRESS_BUILD > 1 instead.

hvlad · 2024-11-18T12:43:32Z

Checks in files must be changed to #if defined(COMPRESS_BUILD) && COMPRESS_BUILD > 1 instead.

Cite syntax rule that requires it. I found none. And it builds will all supported compilers as is.

https://en.cppreference.com/w/cpp/preprocessor/conditional:

#if, #elif
...
After all macro expansion and evaluation of defined, __has_include expressions, any identifier which is not a boolean literal is replaced with the number 0 (this includes identifiers that are lexically keywords, but not alternative tokens like and).

C rules is more explicit:
https://en.cppreference.com/w/c/preprocessor/conditional:

#if, #elif

The expression is a constant expression, using only constants and identifiers, defined using #define directive. Any identifier, which is not literal, non defined using #define directive, evaluates to 0 .

hvlad · 2024-11-18T12:44:36Z

Also, I have no MSYS2 env to check this changes and have no plans to install it.

MSYS2 is installed in GitHub workers. It should be possible to add this build to workflows.

Are you volunteer to add it ?

aafemt · 2024-11-18T12:49:06Z

Are you volunteer to add it ?

Adriano is keen of bird languages and he is only one who understand YML. I have no time to learn it.

raedrizqie · 2024-11-18T13:07:05Z

Here, COMPRESS_DEBUG is guarded.. so should other places
https://github.com/FirebirdSQL/firebird/blob/933ac7bf192b1da2acead0b3d1a36ee27c3abe92/src/burp/mvol.cpp#L461-467

#ifdef COMPRESS_DEBUG
			fprintf(stderr, "Inflated data %d\n", buffer_length - strm.avail_out);
#if COMPRESS_DEBUG > 1
			for (unsigned n = 0; n < buffer_length - strm.avail_out; ++n) fprintf(stderr, "%02x ", buffer[n]);
			fprintf(stderr, "\n");
#endif
#endif

hvlad · 2024-11-18T13:31:05Z

Here, COMPRESS_DEBUG is guarded.. so should other places

I don't think so, sorry ;)

The probem is that you require changes that not specified by standard (at least I can't find such requirements) and such kind of code could be added later - because it is fully legal, AFAIU.
And it is still not clear why all existing builds (incliding GCC's ones) have no problem with this code.

aafemt · 2024-11-18T13:33:20Z

And it is still not clear why all existing builds (incliding GCC's ones) have no problem with this code.

Because they have no -Wundef in command line.

hvlad · 2024-11-18T13:34:54Z

Try this, for example: https://onecompiler.com/cpp/42ygfm9mk
I have check it also with
https://www.onlinegdb.com/online_c++_compiler
https://cpp.sh/
https://www.programiz.com/cpp-programming/online-compiler/
and nobody complains

hvlad · 2024-11-18T13:37:31Z

And it is still not clear why all existing builds (incliding GCC's ones) have no problem with this code.

Because they have no -Wundef in command line.

So, why it is required for MinGW ?

raedrizqie · 2024-11-18T13:42:46Z

It is from the old prefix.mingw

I can just remove -Wundef from it then :)

aafemt · 2024-11-18T13:43:23Z

So, why it is required for MinGW ?

As any warning-related option it is not required. I added it into prefix.mingw along with a lot of others because I consider usage of undefined macros and other "allowed code" as a bad style that can lead to unnoticed bugs.

hvlad · 2024-11-18T13:43:37Z

And it is still not clear why all existing builds (incliding GCC's ones) have no problem with this code.

Because they have no -Wundef in command line.

And you wrong again.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html:

-Wundef

Warn if an undefined identifier is evaluated in an #if directive. Such identifiers are replaced with zero.

It is definitely not about compiler errors, as @raedrizqie said:

For inet.cpp, protocol.cpp and remote.cpp, GCC will error out because COMPRESS_DEBUG is not defined.

hvlad · 2024-11-18T13:44:14Z

It is from the old prefix.mingw

I can just remove -Wundef from it then :)

Ok for me ;)

raedrizqie · 2024-11-18T13:44:33Z

So, why it is required for MinGW ?

As any warning-related option it is not required. I added it into prefix.mingw along with a lot of others because I consider usage of undefined macros and other "allowed code" as a bad style that can lead to unnoticed bugs.

yes.. indeed

hvlad · 2024-11-18T13:54:11Z

About pathtools.cpp (and .h).
Formally it is external code and should be put into /extern, AFAIU
But it would be too much for just a single .cpp/.h file, IMHO.
Anyway I should ask the team - is it not against our rules (maybe implicit ones) ?

After this will be ruled, I accept changes in /src.

But /builds and /extern should be reviewed by someone who better knows it.
@AlexPeshkoff ?

AlexPeshkoff · 2024-11-18T14:58:43Z

Will do.

AlexPeshkoff · 2024-11-18T15:17:39Z

configure.ac


 dnl Check for functions
+if test $PLATFORM != win32; then
 AC_CHECK_FUNCS(gettimeofday)


Functional tests should better avoid OS-dependence. What's wrong with gettimeofday?

IIRC gettimeofday implementation in mingw is different, and compilation will fail..

No, only MSVC has problem with gettimeofday().

I got this error with gettimeofday:

2024-11-18T17:35:02.9691283Z C:/_/B/src/build-CLANG64/src/extlib/UdfBackwardCompatibility.cpp:282:3: error: use of undeclared identifier 'timeMutex' 2024-11-18T17:35:02.9933433Z 282 | timeMutex.enter(); 2024-11-18T17:35:03.0000134Z | ^ 2024-11-18T17:35:03.0078661Z C:/_/B/src/build-CLANG64/src/extlib/UdfBackwardCompatibility.cpp:290:3: error: use of undeclared identifier 'timeMutex' 2024-11-18T17:35:03.0221334Z 290 | timeMutex.leave(); 2024-11-18T17:35:03.0245170Z | ^ 2024-11-18T17:35:03.0249640Z 2 errors generated.

I see no relation between this error and gettimeofday. This is an ordinary bug in a dead code.

configure.ac

extern/ttmath/ttmathtypes.h

builds/install/arch-specific/mingw/fb_config.in

builds/posix/Makefile.in

builds/posix/Makefile.in.examples

AlexPeshkoff · 2024-11-19T07:40:03Z

On 11/18/24 20:25, Raed Rizqie wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In builds/posix/Makefile.in.examples <#8321 (comment)>: > @@ -91,7 +99,7 @@ EMPLOYEE_DB= $(EXAMPLES_DEST)/employee.fdb FINAL_EMPDB= $(EXAMPLES_FB)/empbuild/employee.fdb INTLEMP_DB= $(EXAMPLES_DEST)/intlemp.fdb -EXTAUTH_PLUGIN= $(EXAMPLES_FB)/prebuilt/libfbSampleExtAuth.$(SHRLIB_EXT) +EXTAUTH_PLUGIN= $(EXAMPLES_FB)/prebuilt/$(LIB_PREFIX)fbSampleExtAuth.$(SHRLIB_EXT) This is to mimic MSVC build, where the example dlls doesn't have |lib| prefix

Ahh - OK

AlexPeshkoff · 2024-11-19T08:49:06Z

On 11/18/24 20:56, Raed Rizqie wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In configure.ac <#8321 (comment)>: > @@ -1089,6 +1090,7 @@ fi AC_LANG_POP(C++) dnl Check for functions +if test $PLATFORM != win32; then AC_CHECK_FUNCS(gettimeofday) I got this error with |gettimeofday|: |2024-11-18T17:35:02.9691283Z C:/_/B/src/build-CLANG64/src/extlib/UdfBackwardCompatibility.cpp:282:3: error: use of undeclared identifier 'timeMutex' 2024-11-18T17:35:02.9933433Z 282 | timeMutex.enter(); 2024-11-18T17:35:03.0000134Z | ^ 2024-11-18T17:35:03.0078661Z C:/_/B/src/build-CLANG64/src/extlib/UdfBackwardCompatibility.cpp:290:3: error: use of undeclared identifier 'timeMutex' 2024-11-18T17:35:03.0221334Z 290 | timeMutex.leave(); 2024-11-18T17:35:03.0245170Z | ^ 2024-11-18T17:35:03.0249640Z 2 errors generated. |

This can be fixed but looks like it's really better to avoid use of |gettimeofday() on mingw. |

raedrizqie force-pushed the mingw-support branch from 4659078 to 94e5dcc Compare November 18, 2024 12:14

aafemt reviewed Nov 18, 2024

View reviewed changes

raedrizqie force-pushed the mingw-support branch from 94e5dcc to 034764e Compare November 18, 2024 12:55