Upgrading from bs-platform@9 to rescript@9.1.3: Unbound module Marshal (bisect_ppx?)

1

Hey, we recently tried upgrading from bs-platform to rescript (yay) but unfortunately during cleaning and building, we received the following error:

rescript: [1/3] src/common/bisect_common-Bisect.cmj
FAILED: src/common/bisect_common-Bisect.cmj
File "/Users/peter/Projects/draftbit/builder/node_modules/bisect_ppx/src/common/bisect_common.ml", line 183, characters 3-20:
Error: Unbound module Marshal
FAILED: cannot make progress due to previous errors.
Failure: /Users/peter/Projects/draftbit/builder/node_modules/rescript/darwin/ninja.exe
Location: /Users/peter/Projects/draftbit/builder/node_modules/bisect_ppx/lib/bs
error Command failed with exit code 2.

It sounds like it has something to do with bisect_ppx. That being said, I’m not sure what this means for us moving forward. Any help would be great, thanks!

We rely on the following bs-dependencies:

    "@ahrefs/bs-emotion",
    "@draftbit/re-font-awesome",
    "@draftbit/variable-json",
    "@glennsl/bs-json",
    "bs-js-collections",
    "bs-react-tabs",
    "bs-storybook",
    "bs-tinycolor",
    "bs-webapi",
    "decco",
    "re-classnames",
    "reason-apollo",
    "reason-apollo-hooks",
    "reason-react",
    "reason-react-hooks",
    "relude-parse"

and the following ppx flags:

  "ppx-flags": [
    "decco/ppx",
    [
      "@dylanirlbeck/tailwind-ppx/tailwind-ppx",
      "-path ./src/waterloo/tailwind.css"
    ],
    ["@baransu/graphql_ppx_re/ppx6", "-apollo-mode"],
    "@ahrefs/bs-emotion-ppx/ppx",
    "get_in_ppx/ppx"
  ],
2

See this:

3

It seems it is relude-parse depending on bastset which depends bisect-ppx.

It is not a breaking change from rescript point of view, however, ppx could introduce meta-level dependencies, it generates unused code which uses Marshal, but you need to make it compile even you never use it. bisect-ppx is mostly a code instrumentation tool which should be only enabled in CI.

In retrospect, it is a mistake to introduce ppx in rescript, it makes upgrade so painful, any innocent change could potentially be a breaking change.

Edit: if you don’t mind vendoring, you can comment bisect-ppx relevant fields in bastet, it should just work.

4

I don’t want to comment on the old discussion about Marshall module. If something like this happens again, I would consider creating forks of the key widely used dependencies that will break before releasing the new compiler version, so that each industrial user doesn’t have to discover the breakage individually.

5

how does that work ? If you fork that library but can not publish it under the same name, right?

6

Maybe a temporary organization like “@rescript-compat/” and have the libraries exist there?

1 Like
7

Have @rescript-compat seems to be a good idea
but the work seems to be not small, we will discuss about this and see how we can move forward.
Take the reported case here for example,
a -> b -> c -> d
If we fix d, we also need have c pointed to @rescirpt-compat/d and a, b updated as well.

8

Thanks everyone! I’ll give this a shot and post updates if something doesn’t work out

9

@Hongbo, the statement you are making is not true.

Bisect does not, unless explicitly told to with BISECT_ENABLE=yes, generate any code that uses Marshal (edit: or any code at all), and the error shown by @peterpme does not point into any generated code, but into Bisect’s own code:

Location: /Users/peter/Projects/draftbit/builder/node_modules/bisect_ppx/lib/bs

That’s Bisect’s own file (the JS library part, not even in the Bisect native executable).

Because this is not in generated code or in a native executable, but in a JavaScript library, this kind of error could be caused by any breaking ReScript change, and is not PPX-specific in any way. It’s wrong on a even a technical level to blame this on PPX. I understand you are “against” PPX and accept that, but this is a very hand-wavy and non-engineering way to cast shade on PPX (you have your other legitimate reasons — I am not challenging those other reasons, just this specific post).

The other problem here is that BuckleScript/ReScript has never supported “development” PPXs, which would cut Bisect_ppx out of the dependency chain for non-development users. This was long requested and discussed in this ReScript issue of 2019 vintage. In summary, you ultimately decided not to finish implementing proper PPX flags support. That’s fine as an engineering choice, and I accept it. But, again, the issue isn’t PPX, but ReScript’s choice not to support it, and it’s odd now that you would say this with Bisect, when you so directly participated in the issue before ultimately deciding not to do it.

On Bisect’s side, I continue to fully support ReScript as long as it is technically feasible, i.e. until (if) ReScript truly cripples PPX in some future version.

@peterpme and all, if you have an issue with Bisect or any other projects I work on, please open an issue at the Bisect (or other) repo :slight_smile: I can solve this type of thing within hours if I am not asleep or overwhelmed by some work. Again, it’s odd that you wouldn’t ping the actual repo! Thanks to John Doneth (not sure his name here) for actually letting me know.

ReScript is pretty “against” PPX (and, I accept and understand that) — so if you are using Bisect, do ping the repo instead of (only) posting here. The repo is the right place for support, and this forum probably is not, for a PPX, for the reasons above.

@Hongbo I mean no disrespect, I am merely challenging this post due to an overreaching and technically unfounded generalization. Also, again, I want to repeat, I continue to support ReScript on my end in all my projects that are relevant to it, and am in no way “against” ReScript.

6 Likes
10

That is interesting because graphql-ppx is also using Marshal internally, but doesn’t generate such an error. I don’t see how using Marshal in the code to be compiled to a binary can be a problem? Maybe it’s because ReScript tries to compile native source code that might be included in the package?

11

It looks like this is part of the runtime code in bsconfig.json: https://unpkg.com/browse/bisect_ppx@2.6.1/src/common/bisect_common.ml. And it uses Marshal. Perhaps this directory can be excluded as it doesn’t look like it’s JS runtime code but native code?

12

Hi Anton, your volunteer work for fixing the ppx is appreciated.
Since I am going to take a break from this project so I am not going to dig into the tech details.

What we want is to avoid such things happen in the first place, we recently updated the Roadmap | ReScript Community (rescript-lang.org), we have a ppx retirement plan by the Jan of 2023. With that only application level ppx is supported, so that library level ppx is skipped. This retirement plan actually would fix this use case, since bisect would only make sense when you own the library.

1 Like
13

Exactly, this is Marshal being used in Bisect’s tiny JS library. This part of Bisect is in every technical way like any other ReScript npm package.

So this issue has nothing to do with any code generation, native binaries, etc.

Note that in this case, no code is actually being instrumented or generated, and the JS library is not even being pulled into the JS output of @peterpme’s project. It’s just that the Bisect JS runtime library has to be compiled (and then not used at all), because there is (for now) no way to make Bisect into a true dev-only dependency in ReScript, due to a bsconfig.json limitation. It looks like all PPX will become dev-only anyway, though (thanks @Hongbo), so that should be a vanishing problem.

The file you linked is currently used for both for native and JS. Marshal was just a cheap way for the instrumented code to serialize coverage statistics, kind of like Python pickle.

Against any claims of quality (because of context from earlier posts, and I don’t mean to continue any argument, I just need to state this :slight_smile: ):

  • Using Marshal for this has worked without problems for over a decade on native, and since the beginning on ReScript. It’s been in use since long before I took over the project, and is only broken now due to ReScript’s upstream change (which is fine, and sometimes it’s necessary to adapt downstream projects).
  • We’ve been considering moving away from Marshal for several independent reasons anyway, even on the native side.

So we will just switch away from Marshal in Bisect :slight_smile:

I might vendor Marshal for a quick fix in the meantime, though.

8 Likes
14

Great. Thanks for your work Anton!

1 Like
15

The fix is now out in npm as bisect_ppx@2.6.2.

I took a little break, too.

5 Likes
16

Thank you so much @aantron!

1 Like
17

I assume that you are working off forks of basket, relude, and relude-parse now?