quickjs-rs icon indicating copy to clipboard operation
quickjs-rs copied to clipboard

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

Open branchseer opened this issue 3 years ago • 4 comments

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 https://github.com/bellard/quickjs/pull/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.

branchseer avatar May 10 '21 18:05 branchseer

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.

theduke avatar May 10 '21 18:05 theduke

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.

branchseer avatar May 11 '21 03:05 branchseer

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?

andrieshiemstra avatar Dec 31 '21 09:12 andrieshiemstra

  • 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.

monoclex avatar May 01 '22 07:05 monoclex