-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add clangd wrapper script #17
base: main
Are you sure you want to change the base?
Conversation
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.
Apart from two small changes where I had to hardcode something in the mkosi.clangd
file this is now the setup I got working this evening. I also opened systemd/mkosi#3206 which I think would allow us to make this a lot cleaner and avoid some of the current problems
175385f
to
09d9965
Compare
I now got PS: Well, no it is no longer working. I am not really sure what I changed - maybe it was some fluke due to leftover files from a previous run |
316cfba
to
ff63747
Compare
ff63747
to
39cc63d
Compare
Rebased to use systemd/mkosi#3216 which makes this a decent bit cleaner as the other build scripts no longer get in our way |
modules/kernel/mkosi.clangd
Outdated
# This currently puts the clang-format on the host :/ | ||
# And also DOES NOT WORK... :| | ||
cp kernel/.clang-format $SRCDIR |
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.
We should parse the first argument as the source dir to run clangd in and cd into it before invoking clangd
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.
And omit the --compile-commands-dir
then such that clangd picks it up from the current dir? I am not sure if that really works as well as we hope it does. It also means that we would need to put the compile DB of the kernel under the kernel sources which makes it impossible to reuse the same kernel source for multiple mkosi-kernel projects.
--compile-commands-dir="$BUILDDIR" \ | ||
--path-mappings="\ | ||
$BUILD_SOURCE_MAPPINGS,\ | ||
$BUILD_DIRECTORY/=$BUILDDIR/,\ |
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 should map to $BUILDDIR/$1
if we pass the source dir to run clangd in as the first argument
# This neglects build sources which have an absolute path as a target | ||
# but that would make the jq filter more complex | ||
# and can still added later if someone has a need for this. | ||
BUILD_SOURCE_MAPPINGS="$(jq -r '[ | ||
.BuildSources[] | ||
| .Source + "/='"$SRCDIR"'/" + .Target | ||
] | join(",")' "$MKOSI_CONFIG")" |
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.
We should pass this when invoking mkosi with this script as the beginning. You can map the current working directory to /work/src/$1
if the source dir is passed as the first argument
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 see no reason why we should simply map all dirs for which we have information available. Only mapping a single one will also be difficult because we do not know which of the BuildSources
it is. And passing it via CLI does not work because we need to also know the path outside the build sandbox, not only inside. For systemd this works because there is only a single build source usually configured and that is generally $PWD
on the host
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.
@septatrix I would make the assumption that $PWD
when mkosi.clangd is invoked outside of mkosi is the path outside the sandbox. And the target path inside the sandbox is what's passed in as $1
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.
But the path inside the sandbox will always be one of BuildSources
, same for $PWD
- except for dubious cases where one uses mkosi-kernel as a clangd provider for a different source tree which I think we do not need to support.
Unless I am missing something the current jq filter covers a subset of all mappins which make sense by manually passing them - it just does it automatically for you and for all of them
set -eu | ||
|
||
if [ -z "${MKOSI_CONFIG-}" ]; then | ||
exec mkosi \ |
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.
We shouldn't assume this script is invoked from the mkosi-kernel
source directory. So this should use the BASH_SOURCE
magic to figure out the directory this script is located in and then pass that to --directory
with ../..
to make sure we run mkosi in the right directory
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.
No we indeed shouldn't and that's also why I resolve the path to the build script below. However, I would assume it's more likely that the user wants mkosi to scan the config from the current dir than from mkosi-kernel
(e.g. when including the latter)?
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 think that's the more unlikely scenario.
The more likely scenario is that a user has a separate kernel tree open in their editor and wants to run clangd from mkosi-kernel within that editor workspace. And that means $PWD will not be the mkosi-kernel repository and instead we should chdir() to it with --directory
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.
In that case they can simply create a dummy mkosi.conf
with an include statement for mkosi-kernel or prepend env --chdir ...
to the invocation of the clangd wrapper script. If we always chdir to the mkosi-kernel directory we would make it completely impossible to use mkosi-kernel with Include=
whereas the current state makes that trivial all while your scenario is still quite simple
Regarding |
I have now put the compile commands DB under |
It might also make sense to add the following content to suppress warnings which appear because the
compile_commands.json
is based on GCC and not ClangPS: Some way of getting .clang-tidy in there (or simply referring to the default one provided by the kernel) would also be great