-
Notifications
You must be signed in to change notification settings - Fork 11k
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
build
: generate hex dump of server assets during build
#6661
Conversation
It has been addressed the xz weekend here: But I am happy that finally the code is removed, so looks good |
build
: generate hex dump of server assets on the flybuild
: generate hex dump of server assets during build
Please pay attention that the idea may look simple on the surface, but it's get messy when taking into account that we're not compiling for single target.
Edit: sorry I deleted the |
@phymbert great to read you've already mitigated the issue, I'd missed that commit in the flow.
@ngxson actually I've just hit a snag by using Re/ Windows, I realize I've assumed people build from within WSL (or cross-build from Linux), but would need confirmation. In the absolute worst case we could write our own minimalistic xxd, I suppose. |
14677f1
to
13a36ef
Compare
We cannot assume unfortunately, I have the feeling most windows developper are using Visual Studio.
I would prefer to have a classical npm build then. Also what about: |
build
: generate hex dump of server assets during buildbuild
: generate hex dump of server assets during build
@phymbert possibly inevitable in the long run, would unlock new horizons...
Hah, nifty! Might need some contortions to get something similar to work w/ MSVC, but alternatively... looks like we could do it in cmake: https://stackoverflow.com/questions/11813271/embed-resources-eg-shader-code-images-into-executable-library-with-cmake Which means out of the 3 documented ways to build on Windows:
(so xxd in Makefile for all platforms, and adhoc hexing in the other build scripts 🤞) |
build
: generate hex dump of server assets during buildbuild
: generate hex dump of server assets during build
Good job! The read file as HEX trick in cmake seem to resolve the problem on both mac & windows. For |
This does not sound really awful - we can give it a try if we encounter issues with the proposed PR One idea to make things even more user friendly is to have a minimal |
examples/server/CMakeLists.txt
Outdated
foreach(asset ${SERVER_ASSETS}) | ||
# Portable equivalent of `cd public && xxd -i ${asset} ../${asset}.hpp` | ||
set(input "${CMAKE_CURRENT_SOURCE_DIR}/public/${asset}") | ||
set(output "${CMAKE_CURRENT_BINARY_DIR}/${asset}.hpp") | ||
|
||
string(REGEX REPLACE "\\.| |-" "_" name "${asset}") | ||
file(READ "${input}" data HEX) | ||
string(LENGTH ${data} hex_len) | ||
math(EXPR len "${hex_len} / 2") | ||
string(REGEX REPLACE "([0-9a-f][0-9a-f])" "0x\\1," data ${data}) | ||
file(WRITE "${output}" "unsigned char ${name}[] = {${data}};\nunsigned int ${name}_len = ${len};\n") | ||
endforeach() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this at configure time seems wrong - if the files change between running cmake
(which shouldn't need to be done frequently) and running make
or cmake --build
, they will not be updated.
This could be turned into a separate .cmake script that is run at build time, similar to how we generate build info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, now using an add_custom_command
forking out to scripts/xxd.cmake
Nomic's Kompute fork builds xxd from source (it's a single .c source file) unconditionally: https://github.com/nomic-ai/kompute/blob/d1e3b0953cf66acc94b2e29693e221427b2c1f3f/CMakeLists.txt#L187 |
@cebtenzzre hah great, so we kinda always have xxd after all... (assuming one does
@ggerganov @ngxson I've switched the Makefile to using |
Feel free to merge when ready |
@ochafik |
@sorasoras glancing through it, think it might related to #6797 @jart FYI |
* `build`: generate hex dumps of server assets on the fly * build: workaround lack of -n on gnu xxd * build: don't use xxd in cmake * build: don't call xxd from build.zig * build: more idiomatic hexing * build: don't use xxd in Makefile (od hackery instead) * build: avoid exceeding max cmd line limit in makefile hex dump * build: hex dump assets at cmake build time (not config time)
Currently one needs to manually regenerate asset files in
examples/server
, which:may cause a security risk(reviewers are unlikely to unescape the code - don't wanna be the next target after xz) (edit: already mitigated by ci: server: verify deps are coherent with the commit #6409 as pointed out below)od
is used inMakefile
(unlikexxd
, it should be available on ~all systems, incl. on Windows w/w64devkit
- thanks to busybox), whilecmake
andzig
builds cast their own hex for portability.