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

ci: add bash script to check if llama-impl.h was included erroneously #11617

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mofosyne
Copy link
Collaborator

@mofosyne mofosyne commented Feb 3, 2025

This PR will add some automation to checks against certain includes.

Unsure how to add to make, so the client side checks is only for makefile for now.

This rule is based on Ggerganov's directive in #11596 (review)

@mofosyne
Copy link
Collaborator Author

mofosyne commented Feb 3, 2025

Expected output if #include "llama-impl.h" was added to run.cpp

$ act -j precompile-check
INFO[0000] Using docker host 'unix:///var/run/docker.sock', and daemon socket 'unix:///var/run/docker.sock' 
[Precompile Checks/precompile-check] 🚀  Start image=catthehacker/ubuntu:act-latest
[Precompile Checks/precompile-check]   🐳  docker pull image=catthehacker/ubuntu:act-latest platform= username= forcePull=true
[Precompile Checks/precompile-check]   🐳  docker create image=catthehacker/ubuntu:act-latest platform= entrypoint=["tail" "-f" "/dev/null"] cmd=[] network="host"
[Precompile Checks/precompile-check]   🐳  docker run image=catthehacker/ubuntu:act-latest platform= entrypoint=["tail" "-f" "/dev/null"] cmd=[] network="host"
[Precompile Checks/precompile-check]   🐳  docker exec cmd=[node --no-warnings -e console.log(process.execPath)] user= workdir=
[Precompile Checks/precompile-check] ⭐ Run Main Checkout Repository
[Precompile Checks/precompile-check]   🐳  docker cp src=/home/mofosyne/git/llama.cpp/. dst=/home/mofosyne/git/llama.cpp
[Precompile Checks/precompile-check]   ✅  Success - Main Checkout Repository
[Precompile Checks/precompile-check] ⭐ Run Main Run Forbidden Includes Check
[Precompile Checks/precompile-check]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/1] user= workdir=
| 🔍 Scanning for forbidden includes in ./examples...
| ❌ Forbidden include detected: llama-impl.h
[Precompile Checks/precompile-check]   ❗  ::error file=./examples/run/run.cpp,line=32::Forbidden include: llama-impl.h in ./examples/run/run.cpp at line 32
| ❌ Forbidden includes found. Please remove!
[Precompile Checks/precompile-check]   ❌  Failure - Main Run Forbidden Includes Check
[Precompile Checks/precompile-check] exitcode '1': failure
[Precompile Checks/precompile-check] 🏁  Job failed
Error: Job 'precompile-check' failed

@mofosyne
Copy link
Collaborator Author

mofosyne commented Feb 3, 2025

FYI, this is just a quick proposal. I don't mind if this is not really needed. But we can at least add rules to this bash script as we go along anyway.

@github-actions github-actions bot added script Script related devops improvements to build systems and github actions labels Feb 3, 2025
Copy link
Collaborator

@ericcurtin ericcurtin left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me

@mofosyne mofosyne requested a review from ggerganov February 3, 2025 11:51
Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

Yup, it's not really necessary since this is easy to notice during reviews. But I agree we can probably expand the pre-compile checks with more useful logic in the future.

Just fix the indentation to 4 spaces since this is what most scripts use. And the Makefile is not relevant anymore, so no need to update it in the future.

@slaren
Copy link
Collaborator

slaren commented Feb 3, 2025

This shouldn't be possible without adding an include directory. If the llama.cpp src directory is being available to the examples, that needs to be fixed.

@slaren
Copy link
Collaborator

slaren commented Feb 3, 2025

This is the problem:

target_include_directories(llama PUBLIC . ../include ../common)

The . include directory should be private, the common directory should be removed entirely.

Copy link
Collaborator

@slaren slaren left a comment

Choose a reason for hiding this comment

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

Fix the include directories instead.

@ericcurtin
Copy link
Collaborator

This shouldn't be possible without adding an include directory. If the llama.cpp src directory is being available to the examples, that needs to be fixed.

This is the best fix

@mofosyne
Copy link
Collaborator Author

mofosyne commented Feb 3, 2025

Fix the include directories instead.

Okay let's try that on #11631

Do we want any other precompile checks? If not then we could close this PR... or have it committed but as a stub for future development


edit: CI failed with the change proposed by Slaren in #11631 so this PR will still be useful until then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops improvements to build systems and github actions script Script related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants