-
Couldn't load subscription status.
- Fork 388
Dependency path wrapping + cleanup #2325
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
base: main
Are you sure you want to change the base?
Conversation
|
There is a question about include-paths of dependencies. Do we strictly want all dependencies to be placed in a subdirectory (unless, of course, this is already done upstream)? This seems to be a bit inconsistent. Tests are good. |
|
The paths for |
|
|
066f6fa to
1e78d35
Compare
|
Rebased to fix merge conflicts, but there's still the question of whether we want to strictly enforce including headers coming from dependency as |
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 started reviewing this, but the changes are mixed with indentation updates (mainly due to added "let") and renamed files, making it hard to see your actual changes. Please move cleanup/formatting to a separate PR that only focusses on this
| # most clangd configurations and editors will look in ./build/, but this just makes it easier to find for some niche edge cases | ||
| ln -sfn "${buildpath}/compile_commands.json" "$IOS_SRC/compile_commands.json" | ||
| # ln -sfn "${buildpath}/compile_commands.json" "$IOS_SRC/compile_commands.json" |
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.
If no longer needed, just remove the line?
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.
There are some issues caused by reusing the same build/ directory between IncludeOS, tests and the example unikernel. I left the line around until this is properly fixed so it's easy to uncomment for now.
In order to more easily get clangd happy, I've set up
${dependency}.includeand${dependency}.libtargets to alldeps/(except lest, which is header-only. We should consider precompiling it), instead of requiring to guess these paths every time they're used.I've also nixified
deps/libfdt. Prior to this, we were hoping it was installed on the system, and somehow that was ok. Now we're properly depending on it.Lastly, I've patched
s2n-tlsso that it actually compiles. It's still not linking due to-lgcc_eh, due to static builds... but better something than nothing.