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

quickjs-ng: new wrap #1865

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

quickjs-ng: new wrap #1865

wants to merge 14 commits into from

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Jan 10, 2025

Not ready for merge.


Add new wrap for quickjs-ng

cc @KaruroChori for some help and review

@trim21 trim21 force-pushed the quick-ng branch 5 times, most recently from 5792666 to 01fcdf5 Compare January 10, 2025 19:26
@trim21 trim21 force-pushed the quick-ng branch 6 times, most recently from 87514cf to e44653c Compare January 10, 2025 19:41
@trim21 trim21 marked this pull request as ready for review January 10, 2025 19:42
@trim21 trim21 changed the title wip: quickjs-ng quickjs-ng: new wrap Jan 10, 2025
quickjs_lib = static_library('quickjs', lib_src, dependencies: deps)

quickjs_ng_dep = declare_dependency(
include_directories: include_directories('.'),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good idea.
It exposes all headers, creating name collisions downstream.
I encountered the same problem when adapting other projects like tcc or mongoose.
The trick I used to avoid this, is like what I have done in here: https://github.com/lazy-eggplant/vs.http/tree/master/subprojects/packagefiles/mongoose
Basically I create a meson-include folder, and add symbolic links to the required headers in there.
However, this had a bug up until recently mesonbuild/meson#14073 so it would not work with older versions :(.

Copy link
Contributor Author

@trim21 trim21 Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which means there is no available meson release support this?

In this case looks like I should not do this, or I need set meson version limit to >1.6.1 ....

Copy link

@KaruroChori KaruroChori Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only other alternatives I can see are

  • to fork the project & better organize files so that we can have sane search paths
  • Add some intermediate target which makes those symlinks, and define it as dep for the actual targets. Basically the folder gets patched while building and not by wrap applying the patch. It is not a sane approach, but it would support older versions of meson.
  • keep the . and accepting things will break downstream when building, in scenarios which are quite obscure and hard to predict.

For context, a file config.h in tinycc was creating problems in one of my projects, but only on macos-14 with clang-19. I wasted way to many hours to figure that out. Basically you would encounter this sorts of issues :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only public header of quichjs-ng, I think we should be able to just copy this file to subprojects/packagefiles/quickjs-ng/include/ to avoid this?

Copy link

@KaruroChori KaruroChori Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is possible I guess. It makes the process of maintaining the patch more annoying than what it should be. But it would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying option 2 and try to make it copy/symlink header file to include/ but looks like it's not possible at generate time and it will require downstream users to try to build project once to make these intermediate target to be executed.

If someone just run meson setup there is no target will be executed.

Copy link
Contributor Author

@trim21 trim21 Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should work? I add a file to redirect the include https://github.com/mesonbuild/wrapdb/pull/1865/files#diff-4249ae485f283cd937be82c135e3137bf864fe201cb7e312d14debcd93f8f13f

using a symlink doesn't look very portable

@KaruroChori
Copy link

KaruroChori commented Jan 11, 2025

We might also generate the pkg for the library. Many which are distributed in wrapdb do.
For reference if needed.

@trim21
Copy link
Contributor Author

trim21 commented Jan 11, 2025

We might also generate the pkg for the library. Many which are distributed in wrapdb do. For reference if needed.

I add pkg-config support, but I'm not sure if it will work with current dummy include? I hardly know pkg-config

@trim21
Copy link
Contributor Author

trim21 commented Jan 13, 2025

is it possible to decompress tar as quickjs-ng instead of default quickjs? In case someone has both wrap for quickjs and quickjs-ng

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.

2 participants