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

Serialization format 23 for nqp-JS #655

Merged
merged 1 commit into from
Aug 20, 2020
Merged

Serialization format 23 for nqp-JS #655

merged 1 commit into from
Aug 20, 2020

Conversation

nwc10
Copy link
Contributor

@nwc10 nwc10 commented Aug 16, 2020

nqp-JS is currently a cross-compiler, and so needs to be able to read the serialization output of MoarVM. As of Feb 2020, commit 9709537d90d61529 the MoarVM serialization format was bumped from version 22 to version 23, as it now also serializes closure names.

This broke the nqp-JS build, but it seems that no-one noticed.

This fixes the build at 2020-07 (the current release), but unfortunately additional fixes are needed for changes now made on master. (report coming soon)

With this commit atop 2020-07 I still see these test failures:

Test Summary Report
-------------------
t/nqp/060-bigint.t                   (Wstat: 0 Tests: 157 Failed: 2)
  Failed tests:  96-97
t/nqp/100-dispatcher.t               (Wstat: 0 Tests: 16 Failed: 3)
  Failed tests:  6, 10, 14
t/nqp/114-pod-panic.t                (Wstat: 0 Tests: 0 Failed: 0)
  Parse errors: Bad plan.  You planned 1 tests but ran 0.
t/hll/06-sprintf.t                   (Wstat: 0 Tests: 284 Failed: 12)
  Failed tests:  40, 66-76
  Parse errors: Bad plan.  You planned 294 tests but ran 284.
Files=116, Tests=11601, 236 wallclock secs ( 2.54 usr  0.42 sys + 303.40 cusr 21.92 csys = 328.28 CPU)
Result: FAIL

t/nqp/060-bigint.t and t/hll/06-sprintf.t were both also failing in the same way immediately prior to MoarVM commit 9709537d90d61529.

t/nqp/100-dispatcher.t fails new tests added after later by commit aadea88 -- Add nextdispatcherfor/takenextdispatcher ops. Specifically it fails with Trace: NYI: unimplemented QAST::Op nextdispatcherfor and Trace: NYI: unimplemented QAST::Op takenextdispatcher

t/nqp/114-pod-panic.t fails as a result of

commit c4fcc6438d8d6061dcd3ae431d2771f7c58c4c5d
Date:   Fri May 29 20:45:03 2020 +0200

    Fix t/nqp/114-pod-panic with a relocated `nqp` executable

    As `nqp::execname` is an op code not available on JVM and thus resulting
    in a compile time error, a quite dirty workaround is used. We hide the op
    in an eval and retrieve the value via `die`.

    Also there is no sane way to retrieve the current nqp executable name
    available on the JVM, so skip that test there.

 t/nqp/114-pod-panic.t | 49 ++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

It's unclear what the correct fix for that should be - clearly nqp doesn't work totally cleanly as a cross-compiler.

nqp-JS is currently a cross-compiler, and so needs to be able to read the
serialization output of MoarVM. As of Feb 2020, commit 9709537d90d61529:
    Serialize the names of closures

    The name is held per code object, meaning different closures may carry a
    different name. This is notably true of auto-generated `proto` methods.
    However, we did not preserve this during serialization, meaning that
    Raku language users could see things like AUTOGEN showing up in both
    backtraces and introspection. Fix this by serializing the name along
    with the closure. This, one a spectest is added, will let us resolve
    rakudo/rakudo#3323.

the MoarVM serialization format was bumped from version 22 to version 23,
as it now also serializes closure names.

This broke the nqp-JS build, but it seems that no-one noticed.

This fixes the build at 2020-07 (the current release), but unfortunately
additional fixes are needed for changes now made on master.
Copy link
Contributor

@jnthn jnthn left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me; will merge it so the PR doesn't bitrot.

@jnthn jnthn merged commit dc1839b into master Aug 20, 2020
@jnthn jnthn deleted the nqp-js-S23 branch August 20, 2020 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants