Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix release CI job #936

Merged
merged 1 commit into from
Mar 1, 2025
Merged

Conversation

bnoordhuis
Copy link
Contributor

Apparently pattern: * is not valid syntax. I think we can do without patterns because we want to publish all artifacts anyway.


Gleaned from https://github.com/actions/download-artifact/blob/main/README.md#download-all-artifacts

Apparently `pattern: *` is not valid syntax. I think we can do without
patterns because we want to publish all artifacts anyway.
@chqrlie
Copy link
Collaborator

chqrlie commented Mar 1, 2025

Why not make amalgation a Makefile target so it can be tested on our local machines ? CI would just invoke make amalgation and copy the target file where appropriate...
It seems the contents of the header files are duplicated in the amalgated source file, which is wasteful.

@saghul
Copy link
Contributor

saghul commented Mar 1, 2025

Agree on the Makefilw target.

The whole point of the amalgamated build is to have just one C and one header file so yes they'll be part of the C file, except quickjs.h.

@bnoordhuis
Copy link
Contributor Author

Why not make amalgation a Makefile target so it can be tested on our local machines ?

No particular reason. Pull request welcome.

It seems the contents of the header files are duplicated in the amalgated source file, which is wasteful.

Deliberate design decision mentioned in the linked issue. An amalgamated build's key selling point is that it's easy. Drop in file, compile, done; no fudging with include paths, or other onerous make-work.

@bnoordhuis bnoordhuis merged commit 6d4667e into quickjs-ng:master Mar 1, 2025
61 checks passed
@bnoordhuis bnoordhuis deleted the fix-rel-ci-maybe branch March 1, 2025 16:39
@chqrlie
Copy link
Collaborator

chqrlie commented Mar 1, 2025

It seems the contents of the header files are duplicated in the amalgated source file, which is wasteful.

Deliberate design decision mentioned in the linked issue. An amalgamated build's key selling point is that it's easy. Drop in file, compile, done; no fudging with include paths, or other onerous make-work.

I am well aware that an amalgamated build is a single file (or possibly 2 files including a header file). My point is the token header files included are duplicated in the amalgamated source file: each #include directive is replaced with the contents of the corresponding file, which is necessary for each instance of the files that undergo multiple inclusion, but it is kind of sloppy to not preprocess them for this purpose. No big deal ATM.

@bnoordhuis
Copy link
Contributor Author

it is kind of sloppy to not preprocess them for this purpose

Seems like complicating things for no practical gain. s/sloppy/straightforward/

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 1, 2025

it is kind of sloppy to not preprocess them for this purpose

Seems like complicating things for no practical gain. s/sloppy/straightforward/

Yes, I agree it is no big deal.

Another issue is more problematic: the internal APIs from cutils.c and other utility functions should be made static in the amalgamated source to prevent name clashes with the project it is linked to. I assume the amalgamated source is not compiled into a library so all the non static symbols will be visible.

This could be done using the preprocessor with a JS_INTERNAL storage class for such symbols that would expand to nothing in the regular case and to static in the amalgamated source, but it is ugly.

A cleaner solution would require a more elaborate concatenator that would add the static keyword on all symbols not explicitly tagged as JS_EXTERN.

@bnoordhuis
Copy link
Contributor Author

Good point. Tracking in #940.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants