Add support for MinGW - Githubissues

hvlad commented 5 days ago

With all respect to the job done:

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

raedrizqie commented 5 days ago

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 commented 4 days ago

@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 commented 4 days ago

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 commented 4 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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

aafemt commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

It is from the old prefix.mingw

I can just remove -Wundef from it then :)

aafemt commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

It is from the old prefix.mingw

I can just remove -Wundef from it then :)

Ok for me ;)

raedrizqie commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

Will do.

AlexPeshkoff commented 3 days ago

On 11/18/24 20:25, Raed Rizqie wrote:

@.**** commented on this pull request.

In builds/posix/Makefile.in.examples https://github.com/FirebirdSQL/firebird/pull/8321#discussion_r1846984010:

@@ -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 commented 3 days ago

On 11/18/24 20:56, Raed Rizqie wrote:

@.**** commented on this pull request.

In configure.ac https://github.com/FirebirdSQL/firebird/pull/8321#discussion_r1847025181:

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

|

FirebirdSQL / firebird

Add support for MinGW #8321

if, #elif

if, #elif