Skip to content

Reorder headers to fix MingW broken build #16396

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Reorder headers to fix MingW broken build #16396

wants to merge 1 commit into from

Conversation

suvayu
Copy link

@suvayu suvayu commented Feb 25, 2025 •
edited
Loading

  • The DUCKDB_API macro is defined in duckdb.h, so it should be
    included before any other C-API functions.

This patch moves the capi_internal.hpp header to the top, which
fixes #13911. However I'm not sure if this is expected, or if
there is an underlying macro bug that is showing itself like this.
Any comments will be helpful.

  • disable clang-format arround capi_internal.hpp to prevent automatic
    reordering

I also disabled automatic reordering by clang-format for this
header. I hope this is acceptable.

@suvayu
capi/aggregate_function-c.cpp: reorder headers to fix duckdb#13911
85ff3ac 
- the `DUCKDB_API` macro is defined in duckdb.h, so it should be
  included before any other C-API functions
- disable clang-format arround capi_internal.hpp to prevent automatic
  reordering
@Mytherin Mytherin mentioned this pull request Feb 25, 2025
Merged
@Mytherin
Copy link
Collaborator

Thanks for the PR and the extensive investigation!

This fix is more of a band-aid fix than a full solution - the order of includes should not matter - but because of this investigation I managed to find what I believe to be the real cause of the issue. The DUCKDB_API is defined in two places (winapi.hpp and duckdb.h). It seems that these definitions have diverged. In duckdb.h:

#ifndef DUCKDB_API
#ifdef _WIN32
....
#endif
#endif

In winapi.hpp

#ifndef DUCKDB_API
#if defined(_WIN32) && !defined(__MINGW32__)
....
#endif
#endif

This essentially means that including winapi.hpp before duckdb.h on MinGW prevents symbols from being exported. The aggregate functions happen to have introduced this problem through the include order (where it worked by accident before). And this is also why re-ordering the includes fixes the issue.

The correct fix here is to align these definitions and export symbols again for MinGW - i.e. by making winapi.hpp contain the same definition as duckdb.h. The only question remaining is - this was likely changed for a reason - so does this break anything else?

I've opened a PR for this here - #16397 and am also running some extra CI to see if this breaks anything else in our builds.

@suvayu
Copy link
Author

suvayu commented Feb 25, 2025

I've opened a PR for this here - #16397 and am also running some extra CI to see if this breaks anything else in our builds.

Thanks a lot for the quick review!

You are of course right about first investigating why the winapi.hpp macro changed in the first place. I'll follow the other PR.

Thanks a lot for taking this up 😃

@suvayu suvayu closed this Feb 25, 2025
@suvayu suvayu deleted the mingw_broken_build branch February 25, 2025 23:54
Mytherin added a commit that referenced this pull request Feb 26, 2025
@Mytherin
Rename DUCKDB_API to DUCKDB_C_API for duckdb.h (#16397)
b6d9eab 
Follow-up fix from #16396 - thanks [suvayu](https://github.com/suvayu)
for all of the work on figuring this out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows CI for the Julia Pkg errors with 'Could not load symbol "duckdb_vector_size"'
2 participants
@suvayu @Mytherin