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

build(c): Fix linker error with SQLite tests #2260

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

WillAyd
Copy link
Contributor

@WillAyd WillAyd commented Oct 18, 2024

Noticed this when compiled via meson:

$ meson setup builddir -Dtests=true -Dsqlite=true
$ meson compile -C builddir

...

/usr/bin/ld: driver/sqlite/adbc-driver-sqlite-test.p/sqlite_test.cc.o: in function `SqliteStatementTest_SqlIngestUInt64_Test::TestBody()':
sqlite_test.cc:(.text+0x802): undefined reference to `void adbc_validation::StatementTest::TestSqlIngestType<unsigned long>(ArrowType, std::vector<std::optional<unsigned long>, std::allocator<std::optional<unsigned long> > > const&, bool)'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

I'm not sure why the CMake config is OK with this, likely some magic I don't understand. However, it makes sense to me that the linker is complaining; the adbc_validation.h header declares this template but the definition is tucked away in adbc_validation_statement.cc. So I don't think the TU for sqlite_test.cc would have access to that (?) without moving the definition to the header.

Alternatively we could provide an explicit template instantiation for the type (uint64_t) in adbc_validation_statement.cc, though I don't think that is inline with the intent of this class

@github-actions github-actions bot added this to the ADBC Libraries 15 milestone Oct 18, 2024
@@ -491,6 +491,14 @@ class StatementTest {
const char* timezone);
};

template <typename CType>
void StatementTest::TestSqlIngestType(ArrowType type,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also TestSqlIngestNumericType and TestSqlIngestTemporalType that would be affected by the same issue, although these aren't actually used by any subclasses.

Maybe these should be private methods?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm if not used by subclasses we could keep them in the CC file to hopefully slim down compilation times a tiny bit (though header discipline etc is probably pretty bad anyways)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm if not used by subclasses we could keep them in the CC file

Do you mean to make these free-standing functions in the module? I think the challenge there is that they ultimately wrap TestSqlIngestType, which relies on some of the class members

Copy link
Member

Choose a reason for hiding this comment

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

I mean that private methods would be fine but ideally we can keep the body in the CC file and not in the header? Or is that not doable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are talking about TestSqlIngestType we can move this back to the cc file if we explicitly instantiate the template types that subclasses can use there as well.

The other methods already have their implementations in the cc module

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry I should actually look into things before I comment...TestSqlIngestNumericType etc are already in the CC file so we can keep them as-is/you can mark them private if you think that helps, but otherwise let's merge this

@lidavidm
Copy link
Member

I think I meant to do the explicit instantiations so that we could keep the body out of the header, though this works too.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks

@@ -491,6 +491,14 @@ class StatementTest {
const char* timezone);
};

template <typename CType>
void StatementTest::TestSqlIngestType(ArrowType type,
Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry I should actually look into things before I comment...TestSqlIngestNumericType etc are already in the CC file so we can keep them as-is/you can mark them private if you think that helps, but otherwise let's merge this

@lidavidm lidavidm merged commit 0d0ce79 into apache:main Oct 22, 2024
66 checks passed
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.

2 participants