Add support for MinGW

[raedrizqie]

[hvlad]

With all respect to the job done:

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

[raedrizqie]

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]

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

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]

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]

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]

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]

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]

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]

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

[aafemt]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

It is from the old prefix.mingw

I can just remove -Wundef from it then :)

[aafemt]

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]

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]

It is from the old prefix.mingw

I can just remove -Wundef from it then :)

Ok for me ;)

[raedrizqie]

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]

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]

Will do.