sycl : Re-add erroneously removed -fsycl from GGML_EXTRA_LIBS

[joeatodd]

• I have read the contributing guidelines
• Self-reported review complexity:
  • Low
  • Medium
  • High

Fix for #8665 introduced in #8644.

[joeatodd]

@mudler this fixed the issue for me locally - can you confirm?

@airMeng - this is a small hotfix for a bug introduced by my previous PR.

[airMeng]

It is a reminder that we shall check whether build shared libs both :)

[airMeng]

llama.cpp/.devops/llama-server-intel.Dockerfile

Line 17 in b115105

cmake -B build -DGGML_SYCL=ON -DCMAKE_C_COMPILER=icx -DCMAKE_CXX_COMPILER=icpx -DLLAMA_CURL=ON ${OPT_SYCL_F16} && \

can you update the scripts here so we can avoid the similar issues?

[mudler]

@mudler this fixed the issue for me locally - can you confirm?

@airMeng - this is a small hotfix for a bug introduced by my previous PR.

Seems to work so far! can't give a full ack as now I have another issue caused by another recent change, so the build fails in another spot 😓

If this gets merged before I fix the other issues I'll be checking this out and open up follow-ups if having still problems 👍

[mudler]

Thank you for the quick fix @joeatodd !

[joeatodd]

It is a reminder that we shall check whether build shared libs both :)

I agree - need to build statically in CI.

can you update the scripts here so we can avoid the similar issues?

I'm happy to make a change to catch issues like this. I see a couple of options:

1. Add an ARG which controls -DBUILD_SHARED_LIBS to either llama-server-intel.Dockerfile or llama-cli-intel.Dockerfile, and build that docker image twice as part of the CI.
2. Make either llama-server-intel.Dockerfile or llama-cli-intel.Dockerfile always build static libs.

I imagine option 1. might be unpopular since it implies extra CI resources. Option 2 implies no extra CI jobs, but possible confusion when one fails and the other doesn't. Perhaps a comment in the Dockerfile to draw attention to the static/shared build option?

@airMeng what do you think?

[mudler]

I can confirm now that's fixing the issue on my side 👍 Thanks @joeatodd !

[joeatodd]

@airMeng I'll merge this for now since it's a hotfix, and I can raise a separate PR for testing static builds once we've agreed the best approach.

[airMeng]

It is a reminder that we shall check whether build shared libs both :)

I agree - need to build statically in CI.

can you update the scripts here so we can avoid the similar issues?

I'm happy to make a change to catch issues like this. I see a couple of options:

1. Add an ARG which controls -DBUILD_SHARED_LIBS to either llama-server-intel.Dockerfile or llama-cli-intel.Dockerfile, and build that docker image twice as part of the CI.
2. Make either llama-server-intel.Dockerfile or llama-cli-intel.Dockerfile always build static libs.

I imagine option 1. might be unpopular since it implies extra CI resources. Option 2 implies no extra CI jobs, but possible confusion when one fails and the other doesn't. Perhaps a comment in the Dockerfile to draw attention to the static/shared build option?

@airMeng what do you think?

I prefer option2. llama-server-intel.Dockerfile for shared lib and llama-cli-intel.Dockerfile for static. And some logging might be helpful.