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

Upgrades QuickJS to 2021-03-27, and support msvc and i686 targets #114

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

Conversation

branchseer
Copy link

This PR upgrades QuickJS to 2021-03-27, and adds supports for these targets:

  • x86_64-pc-windows-msvc
  • i686-pc-windows-msvc
  • i686-unknown-linux-gnu

What this PR does in detail:

  • Remove QuickJS sources in the repo and reference via git submodules to a fork that supports msvc (https://github.com/patr0nus/quickjs/tree/rs).
    That fork is originated from Improve cross platform comparability for quickjs. bellard/quickjs#49 and applied with the js-tobigint64-overflow.patch, so the patch feature is also no longer needed. I took the liberty of ignoring stack-overflow-signed.patch since tests are passed without it any way. Please let me know if I missed anything.
    Also, if you are interested in merging this PR, please consider maintaining a separate QuickJS fork instead of using mine, as I am not really familiar with what the patches do.
  • Generate the bindings on the fly in build.rs, because the generated bindings would be different between these newly-added targets.
  • Treat JSValue as an opaque type. Use JS_VALUE_* functions to read/write it, because the JSValue is just an alias to u64 under i686 targets.
  • Update github actions to cover all the supported targets.

@theduke
Copy link
Owner

theduke commented May 10, 2021

Hey.

Thanks for this effort, supporting MSVC is very valuable!

Also, if you are interested in merging this PR, please consider maintaining a separate QuickJS fork instead of using mine, as I am not really familiar with what the patches do.

Sadly the patch on the mailing list didn't get any response.

I'm not comfortable with using a fork with so many significant changes as the default option since we would need to maintain and update it, and also thoroughly review it.

What would be fine for me is providing the fork under a separate feature, like unstable-msvc.
(unstable to indicate that this isn't necessarily supported)
Glancing at the code this should be doable by sticking to the new static functions.

So the plan of action would be:

  • Add a new fork-msvc.patch to the sys crate
  • Add a feature to the sys crate that handles applying the patch
  • add a feature to the main crate

Bonus: update build.rs to fail with a descriptive error message on Windows if the feature is not enabled.

@branchseer
Copy link
Author

branchseer commented May 11, 2021

Hi. I'm all for providing the fork under an unstable feature. However I'm not sure about applying patches using the patch command in build.rs, especially when we are supporting msvc. Because unlike msys2, the vs dev environment doesn't provide the patch executable. We would have to ask comsumers of quickjs-rs to install it.

What do you think we add two git submodules? quickjs pointing to a branch of original quickjs with just js-tobigint64-overflow applied, and quickjs-msvc pointing to a branch of the msvc fork with js-tobigint64-overflow applied. I admit it's not ideal since we'll have two different quickjs implementations in the crate.

@andrieshiemstra
Copy link
Contributor

Hi @patr0nus

I'm interessted in using this as an optional feature in my quickjs wrapper project (https://github.com/HiRoFa/quickjs_es_runtime) but i'm a bit concerned about future proofing it.

You seem to be using your own branch of quickjs which is a fork a specific branch of a fork of quickjs (lyqstate/quickiot)

When there is a new release of quickjs by bellard, how how much work would it be to update your branch?

And are you planning/willing to keep your branch up to date or should i fork it mysqlf?

Also, the i686 support means it should work on 32bits raspberry os right?

@monoclex
Copy link

monoclex commented May 1, 2022

  • I took the liberty of ignoring stack-overflow-signed.patch since tests are passed without it any way.

I just ran into #66, which is an issue fixed by this patch. Just happened to pass by here and see this, so figured I'd mention it.

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.

None yet

4 participants