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

server : update js #7670

Merged
merged 1 commit into from
May 31, 2024
Merged

server : update js #7670

merged 1 commit into from
May 31, 2024

Conversation

ggerganov
Copy link
Owner

I think the upstream js code was recently updated causing our CI to fail here:

echo "download js bundle files"
curl https://npm.reversehttp.com/@preact/signals-core,@preact/signals,htm/preact,preact,preact/hooks > $PUBLIC/index.js
echo >> $PUBLIC/index.js # add newline

https://github.com/ggerganov/llama.cpp/actions/runs/9317323911/job/25647386183

Would be nice someone to confirm before merge

@hanishkvc
Copy link
Contributor

@ggerganov @mofosyne

My conclusion is the same. So in the short term, I think updating the index.js in git repo into the new one generated from deps.sh would do.

However In the long run, may be a copy needs to be maintained locally of the js modules, if the reason the deps.sh check was added was to ensure that things dont break if upstream (wrt the js modules) changes. And inturn periodically who ever is maintaining that code, takes a call on pulling in the upstream, after validation.

@phymbert
Copy link
Collaborator

I recall recent changes in the way we build static assets.

@hanishkvc
Copy link
Contributor

hanishkvc commented May 31, 2024

I recall recent changes in the way we build static assets.

Hi @phymbert @ggerganov

Hi phymbert not sure what you meant by static assets, in this case its the external js modules related to preact, which seem to get bundled into index.js file. And looking at the failure, definitely the upstream has changed.

I assume who ever created this flow, added this check so as to be able to cross check, if the upstream has changed wrt what was bundled into index.js. Either way given that the code is picked from the bundled index.js and not from upstream at runtime (ie index.html), I dont think this check should be part of the normal CI flow. Instead this is more a check which should be done by the concerned developer periodically decoupled from the general CI flow, and inturn the bundled in index.js updated once they find that there are no adverse changes in the upstream.

The reason I say above, is because, each time the upstream preact changes, this issue with CI failure will occur, while technically the code in this project is not depending on that upstream in a direct sense.

Rather what I mean is this PR should be merged, and at a later time who ever is currently maintaining the default builtin html+jss code, should potentially look at updating the ci test.

@ggerganov ggerganov merged commit a323ec6 into master May 31, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Review Complexity : Medium Generally require more time to grok but manageable by beginner to medium expertise level server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants