Skip to content

ci: add debug build to sanitzier build matrix #527

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

Merged
merged 1 commit into from
Mar 26, 2023

Conversation

Green-Sky
Copy link
Collaborator

discovered ggml.c does not compile in Debug right now.

@Green-Sky Green-Sky requested a review from anzz1 March 26, 2023 14:27
@Green-Sky Green-Sky marked this pull request as draft March 26, 2023 14:29
@Green-Sky
Copy link
Collaborator Author

Green-Sky commented Mar 26, 2023

for some reason it compiles in ci in debug. looking at why.
edit: good fails.

@Green-Sky Green-Sky marked this pull request as ready for review March 26, 2023 14:42
@Green-Sky
Copy link
Collaborator Author

the ci results: https://github.com/Green-Sky/llama.cpp/actions/runs/4525140706

for reference, the compile error:

/home/runner/work/llama.cpp/llama.cpp/ggml.c:5839:20: error: ‘ne13’ undeclared (first use in this function); did you mean ‘nb13’?
 5839 |     assert(ne03 == ne13);
      |                    ^~~~
/home/runner/work/llama.cpp/llama.cpp/ggml.c:5840:12: error: ‘ne2’ undeclared (first use in this function); did you mean ‘nb2’?
 5840 |     assert(ne2  == ne12);
      |            ^~~
/home/runner/work/llama.cpp/llama.cpp/ggml.c:5841:12: error: ‘ne3’ undeclared (first use in this function); did you mean ‘nb3’?
 5841 |     assert(ne3  == ne13);
      |            ^~~
/home/runner/work/llama.cpp/llama.cpp/ggml.c:5844:12: error: ‘nb00’ undeclared (first use in this function); did you mean ‘nb0’?
 5844 |     assert(nb00 == sizeof(float));
      |            ^~~~
/home/runner/work/llama.cpp/llama.cpp/ggml.c:5852:12: error: ‘ne0’ undeclared (first use in this function); did you mean ‘nb0’?
 5852 |     assert(ne0 == ne01);
      |            ^~~
/home/runner/work/llama.cpp/llama.cpp/ggml.c:5853:12: error: ‘ne1’ undeclared (first use in this function); did you mean ‘nb1’?
 5853 |     assert(ne1 == ne11);
      |            ^~~
/home/runner/work/llama.cpp/llama.cpp/ggml.c:5809:15: warning: unused variable ‘ne10’ [-Wunused-variable]
 5809 |     const int ne10 = src1->ne[0];
      |               ^~~~

@Green-Sky Green-Sky requested review from j-f1, slaren and sw March 26, 2023 14:45
@sw
Copy link
Contributor

sw commented Mar 26, 2023

You could uncomment those lines and add #ifndef NDEBUG around it, I've already done that in a pending PR:

master...sw:llama.cpp:q-refactor#diff-6d9ce99fcb6f51ff76f59e479f6e6fc0bb62edef7442805d7a5bb15b23996b5dR5810-R5820

Why does it say "All checks have passed"? It's not doing the CI checks?

@Green-Sky
Copy link
Collaborator Author

Why does it say "All checks have passed"?

you mean for this pr? that's because it does not build for non code changes in prs...

@Green-Sky
Copy link
Collaborator Author

You could uncomment those lines and add #ifndef NDEBUG around it, I've already done that in a pending PR

make a pr and i will rebase 😄

@sw sw merged commit 34c1072 into ggml-org:master Mar 26, 2023
@anzz1
Copy link
Contributor

anzz1 commented Mar 26, 2023

Why does it say "All checks have passed"? It's not doing the CI checks?

The pull_request target does not have write permissions to the target repo (this repo) so that's why the workflows aren't set as triggers in PR's to avoid confusion of the changed workflow not working. The pull_request_target which isnt used here would have write permissions and should never be used in public repos, though many do not understanding the difference, having such similar names and all.

Workflow changes do trigger for pushes, so if the source repo has enabled actions (like @Green-Sky did) you can see the result of the changed workflow in the source repo where the commit was pushed. This is useful when you make a fork for a patch and want to play around with the actions in your own repo. There is also the workflow_dispatch trigger which simply means you can manually run the workflow for any branch in your own fork repo by going to "Actions -> CI -> Run workflow". I also added the "create new release" checkbox there so releases can be created from feature branches if you want to test making a release in ur fork before making a PR.

@anzz1
Copy link
Contributor

anzz1 commented Mar 26, 2023

To be honest, the "complete build" passing while the sanitizer builds fail can be a bit confusing, but it's necessary for now so that the whole build doesn't get canceled from the sanitizer errors.

It's just a temporary solution though, once we get rid of all the leaks and get all the sanitizer builds to pass, the continue-on-error flags should be removed so new leaks can't be introduced as they will fail the build.

@Green-Sky
Copy link
Collaborator Author

@anzz1 yea, we can tighten down the screws when stuff settles a bit.

@Green-Sky Green-Sky deleted the ci_buildtypes branch May 1, 2023 10:22
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.

3 participants