Skip to content

Conversation

@real-dam
Copy link
Contributor

This is the last one of the "use system $lib" series.


#include "ibase.h"
#include "./Interface.h"
#if defined USE_SYSTEM_BOOST
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is public header file. So I think name must be better FB_USE_SYSTEM_BOOST.

@aafemt
Copy link
Contributor

aafemt commented Sep 15, 2021

Taking into account that C++11 features are fully available, do we still need boost?

avoids potential clashes with third party software which includes
firebird/Message.h
@real-dam
Copy link
Contributor Author

Taking into account that C++11 features are fully available, do we still need boost?

I like the idea of replacing an external dependency with a compiler feature. Hopefully someone more fluent in C++ than me would try it.

@asfernandes
Copy link
Member

Taking into account that C++11 features are fully available, do we still need boost?

We didn't discussed C++11 in public headers yet. I support it.

configure.ac Outdated

LOCAL_BOOST=Y
AC_ARG_WITH(system-boost,
[ --with-system-bools use system-wide boost library instead of embedded copy],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is misspelled (bools).

@asfernandes
Copy link
Member

How that will work for public headers?

Will you put a dependency for boost in firebird-client-dev (or the name you use) package?

Will you deploy fb-boost headers in that package?

Will the user also need to use the define?

@real-dam
Copy link
Contributor Author

How that will work for public headers?

Will you put a dependency for boost in firebird-client-dev (or the name you use) package?

Will you deploy fb-boost headers in that package?

Will the user also need to use the define?

Good question. Requiring that all consumers of firebird/Message.h define USE_SYSTEM_BOOST would be bad.

Perhaps I should reverse the define and rename it to USE_FB_BOOST? This way consumers using the Debian package would only need to have boost around, maybe pulled by a dependency.

That dependency, while technically needed, is not present in firebird3.0 packages which have a similar patch (crudely using system boost and replacing FB_BOOST_xxx with BOOST_xxx in Message.h) and nobody complained for the last 5+ years. I guess some softer form of dependency would be enough.

Will update the PR with USE_FB_BOOST and reverse meaning.

@real-dam real-dam marked this pull request as draft September 16, 2021 19:18
@aafemt
Copy link
Contributor

aafemt commented Sep 16, 2021 via email

@real-dam
Copy link
Contributor Author

Will update the PR with USE_FB_BOOST and reverse meaning.
Oh, but that would mean that any Message.h consumers that have it from non-packaged Firebird would need the define. Not good either.

I think the approach used in fb3 Debian packages is the least evil.

The cleanest way from consumer side would be to have Messages.h with either include <...boost...> (plus a dependency) or include "...boost...", both hard-coded and not controlled by a define.

fb3 debs took one approach at this, patching the installed files.

An approach that controls the output via --with-system-boost would require similar sort of patching, for example via sed. Shouldn't be a problem since this affects only posix builds. Will be kind of ugly, but should work.

The problem with the #ifdef approach is that Message.h is a public header
that is shipped to user systems and should not require defines to be usable
@real-dam real-dam marked this pull request as ready for review September 17, 2021 06:28
@real-dam
Copy link
Contributor Author

This became as good as patching during package build with the added benefit that including it upstream will make the functionality available for other downstream distributions.

@asfernandes asfernandes merged commit 615c034 into FirebirdSQL:master Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants