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

BLOCK_SIZE C Macro collision #6504

Closed
2 tasks done
pedroerp opened this issue Feb 28, 2023 · 4 comments
Closed
2 tasks done

BLOCK_SIZE C Macro collision #6504

pedroerp opened this issue Feb 28, 2023 · 4 comments
Labels

Comments

@pedroerp
Copy link
Contributor

What happens?

Not a bug in duckdb, but we recently bumped into a collision issue in one internal system which ends up including a kernel header (fs.h) that defines BLOCK_SIZE as a C macro, and that ends up conflicting and rewriting Storage::BLOCK_SIZE in duckdb, and thus failing compilation.

I know there are hacky ways to around this (like undefining this macro before including duckdb), but considering BLOCK_SIZE is a fairly general name, any chance you would be willing to accept a PR that renames Storage::BLOCK_SIZE to something like Storage::DUCKDB_BLOCK_SIZE or maybe Storage::block_size?

To Reproduce

Include any library that has a C-style define BLOCK_SIZE before including duckdb.hpp.

OS:

Ubuntu, but reproduces on any.

DuckDB Version:

Any

DuckDB Client:

Any

Full Name:

Pedro Pedreira

Affiliation:

Meta Platforms Inc.

Have you tried this on the latest master branch?

  • I agree

Have you tried the steps to reproduce? Do they include all relevant data and configuration? Does the issue you report still appear there?

  • I agree
carlopi added a commit to carlopi/duckdb that referenced this issue Feb 28, 2023
This should make Storage not visible from duckdb.hpp, so that less conflicts like duckdb#6504
would happen. Broader idea would be that functions tagged DUCKDB_API should be visible
from duckdb.hpp, the rest are just implementation details that can potentially hidden.
@carlopi
Copy link
Contributor

carlopi commented Feb 28, 2023

Hi @pedroerp! I took a look at the issue, and I think I have an alternative solution that might require less changes: move declaration of struct Storage to an 'inner' header, that should not be included from duckdb.hpp.

Could you potentially check if this is of some use, doing something like:

cd duckdb
git remote add carlopi https://github.com/carlopi/duckdb.git
git fetch carlopi
git checkout carlopi/includeStorage

?

[ Here the linked PR: #6505 ]

carlopi added a commit to carlopi/duckdb that referenced this issue Feb 28, 2023
This should make Storage not visible from duckdb.hpp, so that less conflicts like duckdb#6504
would happen. Broader idea would be that functions tagged DUCKDB_API should be visible
from duckdb.hpp, the rest are just implementation details that can potentially hidden.
@pedroerp
Copy link
Contributor Author

pedroerp commented Mar 4, 2023

Hi @carlopi thanks for the quick reply. Looks like this solved the issue. Any chance you could get it merged?

@github-actions
Copy link

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Jul 29, 2023
@github-actions
Copy link

This issue was closed because it has been stale for 30 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants