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

Handle macos SDK in the justfile include_args #184

Merged
merged 3 commits into from
Nov 17, 2024

Conversation

mkatychev
Copy link
Contributor

just lint would return missing header errors for macOS making debugging tricky:

checking file src/scanner.c
5 warnings and 1 error generated.
Error while processing /Users/mkatychev/Documents/tree-sitter/just/src/scanner.c.
./tree_sitter/parser.h:10:10: error: 'stdlib.h' file not found [clang-diagnostic-error]
#include <stdlib.h>
         ^~~~~~~~~~

Added conditional to include_args for handling CPATH to stdlib.h:

include_args := "-Isrc/ -I" + ts_path + "/lib/include -Inode_modules/nan" + if os() == "macos" {
" -I" + `xcrun --sdk macosx --show-sdk-path` + "/usr/include/"
} else {
""
}

dependency checker now own just command:

_check_installed +dep:
#!/bin/sh
set -eau
check_installed() {
printf "checking $1... "
local dep=0
command -v "$1" 1>/dev/null || dep=$?
if [[ "${dep}" == 0 ]]; then
echo "tool $1 found!"
else
echo
echo "tool $1 NOT found. This may be needed for some functionality"
fi
}
for d in {{dep}}; do
check_installed $d
done

Added justfile to .editorconfig:

[justfile]
indent_style = tab
indent_size = 4

printf "checking $1... "
if "$1" --version 2> /dev/null ; then
local dep=0
command -v "$1" 1>/dev/null || dep=$?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this allows checking dependencies that do not have a --version flag

Copy link
Collaborator

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One nit then lgtm

justfile Outdated
Comment on lines 81 to 82


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, double whitespace

justfile Outdated Show resolved Hide resolved
Co-authored-by: Trevor Gross <[email protected]>
justfile Outdated Show resolved Hide resolved
@tgross35 tgross35 merged commit 11b8c43 into IndianBoy42:main Nov 17, 2024
4 checks passed
@tgross35
Copy link
Collaborator

Looks good, thanks for the changes!

setup *npm-args:
#!/bin/sh
set -eau
just _check_installed npm cargo clang clang-tidy clang-format kk
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgross35 I'm surprised this kk passed CI, I wonder if the check was skipped?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check-installed doesn't actually require it be installed, just warn when it isn't. Maybe this should change on CI.

I actually had to adjust this in #186 and removed it there - it was always saying not found, bash syntax with a sh shebang :) I should have read the output better

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 should have tested it myself, should not have pushed the commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants