Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve dependency install and build #10920
Improve dependency install and build #10920
Changes from all commits
bbe6676
0f7ad1d
a268fdb
fdc8276
baf2b99
425a809
50f9065
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love that, I think adding this on a target basis to all targets with
BEFORE
(viavelox_add_library
) should also do the job. I am pretty sure this will not take precedent before default system include paths, as they don't even appear in the compiler invocation but should work for any add brew paths.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah actually not true. According to gcc docs system default paths are searched last, so this should also work in non-brew cases (or linked brew installs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@assignUser I agree that per target is cleaner. But INSTALL_PREFIX also has the dependencies installed via the setup script. There seems no standard way to get the INCLUDE_DIR path for each dependency. Some packages like fmt add INTERFACE_INCLUDE_DIRS to the target (fmt::fmt) property, folly on the other hand defines the
FOLLY_INCLUDE_DIRS
variable. Protobuf and CUDA are currently addinginclude_directories
after resolving the dependency. Should we do that for all the dependencies and add BEFORE?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you mean for the compilation of the dependencies themselves not when we are using them. Hm, yeah
include_directories
seems to be the best way to do that due to it's global nature... though if we haveINSTALL_PREFIX
are we even building any deps from source that are not self-contained/header only?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant when Velox is using the dependencies. I thought about this further and removed the global include_directories for INSTALL_PREFIX. This will only work if we always install dependencies. If some of the dependencies come from brew, this could cause issues if an older version of a package is present in INSTALL_PREFIX.
I added this to fix the fmt header issue. I now explicitly get the fmt include path and add it to include_directories with before tag. That is what you were suggesting too. Can you take another look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my assumption
INSTALL_PREFIX
exits == all deps installed there. And in that case the solution with include_directories (or per target) would work well.Hm,maybe but that also seems like a mis-configuration then? Either use install prefix with all required deps or don't?
I think I liked the previous version more but 🤷 no strong preferences
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good reason to use the previous approach. I reverted to that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This CMake statement for header file orders breaks
duckdb
build, asfmt
(a custom copy) andduckdb re2
(using a different namespace) now are searching headers from${INSTALL_PREFIX}
, instead of its local directory, mentioned in #11058.It seems also causing a build issue for
Arrow
mentioned in #11052.We might need to rethink of a better way for the header include order, if it is really needed.