Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add -export-symbols-regexp to LDFLAGS #162

Merged
merged 1 commit into from Dec 31, 2017

Conversation

paravoid
Copy link
Contributor

As to only export symbols starting with "MMDB_" and in lieu of a proper
version script. This helps by filtering out the new data_pool_* symbols,
which are internal-only and should not exposed to applications using
libmaxminddb.

The change to LDFLAGS would introduce a test failure, as data-pool-t
used those internal functions, but this is addressed by adding
src/data-pool.c to data-pool-t's SOURCES, as suggested by Will Storey.

This resolves #160.

@horgh
Copy link
Contributor

horgh commented Dec 29, 2017

Thanks for making the change!

Weirdly Travis is failing when building on Linux. I've found a workaround, but I'm not sure why it happened. The changes work fine with automake 1.15 (using Ubuntu Artful), but with automake 1.14.1 (Ubuntu Trusty, like the Travis builds), they fail. I can't find anything in the changelog that would explain this behaviour changing in automake, but I have a suspicion that's where the problem is coming from.

I determined a workaround from this bug report where they state that using $() variables in _SOURCES is not valid. However, I can't find such a restriction in the automake docs. There's this, but I don't think it should apply to $(top_builddir):

You can’t put a configure substitution (e.g., ‘@FOO@’ or ‘$(FOO)’ where FOO is defined via AC_SUBST) into a _SOURCES variable. The reason for this is a bit hard to explain, but suffice to say that it simply won’t work.

In any case, the workaround is this:

diff --git a/t/Makefile.am b/t/Makefile.am                                                                                                                                           
index 7e0a464..9983449 100644
--- a/t/Makefile.am
+++ b/t/Makefile.am
@@ -18,7 +18,7 @@ check_PROGRAMS = \
 
 data_pool_t_CFLAGS = $(CFLAGS) -I$(top_srcdir)/src
 data_pool_t_LDFLAGS = $(AM_LDFLAGS) -lm
-data_pool_t_SOURCES = data-pool-t.c $(top_builddir)/src/data-pool.c
+data_pool_t_SOURCES = data-pool-t.c ../src/data-pool.c
 
 threads_t_CFLAGS = $(CFLAGS) -pthread
 

This works with both automake 1.14.1 and automake 1.15.

As to only export symbols starting with "MMDB_" and in lieu of a proper
version script. This helps by filtering out the new data_pool_* symbols,
which are internal-only and should not exposed to applications using
libmaxminddb.

The change to LDFLAGS would introduce a test failure, as data-pool-t
used those internal functions, but this is addressed by adding
src/data-pool.c to data-pool-t's SOURCES, as suggested by Will Storey.

This resolves maxmind#160.
@paravoid
Copy link
Contributor Author

Oh wow, that investigation was great! I updated the PR with your findings and suggestions and it seems to pass the tests now. I get these warnings when building locally:

t/Makefile.am:21: warning: source file '../src/data-pool.c' is in a subdirectory,
t/Makefile.am:21: but option 'subdir-objects' is disabled
automake: warning: possible forward-incompatibility.
automake: At least a source file is in a subdirectory, but the 'subdir-objects'
automake: automake option hasn't been enabled.  For now, the corresponding output
automake: object file(s) will be placed in the top-level directory.  However,
automake: this behaviour will change in future Automake versions: they will
automake: unconditionally cause object files to be placed in the same subdirectory
automake: of the corresponding sources.
automake: You are advised to start using 'subdir-objects' option throughout your
automake: project, to avoid future incompatibilities.

…but the build succeeds, so it should be fine, at least for now.

@horgh
Copy link
Contributor

horgh commented Dec 31, 2017

Thanks for bringing up the warning, I missed that. I think we could silence it by turning on that option, but it looks like the build is fine either way, so let's leave it (that option seems a little buggy from what I read, see below).

While looking at that option, I came across this bug in automake related to the use of $() in _SOURCES. Although I still can't see why the the behaviour changed between 1.14.1 and 1.15, since from reading that thread it won't be fixed until the next release. Oh well!

Thanks again!

@horgh horgh merged commit 65a2686 into maxmind:master Dec 31, 2017
@paravoid paravoid deleted the to-upstream/160 branch January 8, 2021 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

data_pool_* symbol visibility
2 participants