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.

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

Add clangd wrapper script #17

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add clangd wrapper script #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39cc63d
Add clangd wrapper script
septatrix Nov 18, 2024
f0a8c83
Put compile commands DB under kernel $BUILDDIR
septatrix Nov 25, 2024
8c99796
Silence CC options unknown to clangd
septatrix Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions modules/kernel/mkosi.build.chroot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ export KBUILD_BUILD_USER="mkosi"

make $EXTRA -j "$(nproc)" $( ((INCREMENTAL)) && echo modules)

scripts/clang-tools/gen_compile_commands.py -d "$BUILDDIR" -o "$BUILDDIR"/compile_commands.json

septatrix marked this conversation as resolved.
Show resolved Hide resolved
# FIXME: Re-enable when coreutils 9.5 gets into debian testing.
. /usr/lib/os-release
if ! ((INCREMENTAL)) && [ ! "$ID" = "debian" ]; then
Expand Down
81 changes: 81 additions & 0 deletions modules/kernel/mkosi.clangd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
#!/bin/bash
# SPDX-License-Identifier: LGPL-2.1-or-later

set -eux

if [ -z "${MKOSI_CONFIG-}" ]; then
# shellcheck disable=SC2093 # exec not last command
exec mkosi \
Copy link
Owner

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

Copy link
Contributor Author

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)?

Copy link
Owner

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

Copy link
Contributor Author

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

--incremental=strict \
--build-sources-ephemeral=no \
--format=none \
--build-script= \
--build-script="$(realpath "$0")" \
build
septatrix marked this conversation as resolved.
Show resolved Hide resolved
"$@"
fi

if [ -z "${CONFIG-}" ]; then
if [ -f kernel/mkosi.kernel.config ]; then
CONFIG=kernel/mkosi.kernel.config
else
CONFIG=mkosi.kernel.config
fi
fi

collect_files()
{
local file res=

for file; do
case "$file" in
*\~*)
continue
;;
esac
if test -e "$file"; then
res="$res$(cat "$file")"
fi
done
echo "$res"
}

FILE_LOCALVERSION="$(collect_files kernel/localversion*)"
CONFIG_LOCALVERSION=$(sed -n 's/^CONFIG_LOCALVERSION="?\(.*\)"?$/\1/p' "$SRCDIR/$CONFIG" | cut -d '"' -f2)

BUILDDIR="$BUILDDIR/kernel/${FILE_LOCALVERSION}${CONFIG_LOCALVERSION}${LOCALVERSION:-""}"

DISTRIBUTION="$(jq -r .Distribution "$MKOSI_CONFIG")"
RELEASE="$(jq -r .Release "$MKOSI_CONFIG")"
ARCH="$(jq -r .Architecture "$MKOSI_CONFIG")"

BUILD_DIRECTORY="$(jq -r .BuildDirectory "$MKOSI_CONFIG")"
CACHE_DIRECTORY="$(jq -r .CacheDirectory "$MKOSI_CONFIG")"
# 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")"
Comment on lines +54 to +60
Copy link
Owner

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

Copy link
Contributor Author

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

Copy link
Owner

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

Copy link
Contributor Author

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


# clangd searches the directory upwards for a .clang-format file
# but path-mappings are not respected for this procedure.
# So the filenames supplied by the LSP client are using host paths
# which do not exist inside the build sandbox.
# To work around this, we copy the .clang-format file to the root
# such that clangd uses it as a fallback.
cp kernel/.clang-format /
cat > /.clangd <<EOF
CompileFlags:
Add: -Wno-unknown-warning-option
Remove: [--hack*, --ibt,--orc,--retpoline,--rethunk,--sls,--static-call,--uaccess,--link,--module,-mpreferred-stack*,-mindirect-branch*,-fno-allow-store*,-fconserve-stack,-mrecord-mcount,-ftrivial-auto*]
EOF
exec clangd \
--enable-config \
--compile-commands-dir="$BUILDDIR" \
--path-mappings="\
$BUILD_SOURCE_MAPPINGS,\
$BUILD_DIRECTORY/=$BUILDDIR/,\
Copy link
Owner

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

$CACHE_DIRECTORY/$DISTRIBUTION~$RELEASE~$ARCH.cache/usr/include/=$BUILDROOT/usr/include/" \
"$@"