Change bisect_ppx to dev dependency when possible

[andywhite37]

bisect_ppx is a code coverage tool that is only needed at dev time, and not for prod installs. Right now, bucklescript only allows ppxs as normal dependencies, but this is supposed to change sometime in the future. When it's possible to have dev ppx dependencies, move bisect_ppx to a dev dep.

[jihchi]

The relevant ticket: rescript-lang/rescript#3761

[utkarshkukreti]

@andywhite37 What do you think about manually removing the bisect_ppx dependency when doing a release? Relude currently doesn't work on Windows as bisect_ppx does not compile on windows (I've made several other failed attempts in the last 2 months to get this to work).

[andywhite37]

@utkarshkukreti - Yeah, I think that would be fine to do. I'm not crazy about having that dependency in the non-dev dependencies anyway. I'm not going to be able to do it right this second, but we can make a release next week that removes it.

[utkarshkukreti]

@andywhite37 works for me, thanks!

[andywhite37]

@utkarshkukreti - sorry we haven't been able to get to this so far. If you want to submit a PR to remove the dependency, I would be okay with that. It would be nice to make that depenency removal fairly non-invasive, b/c it's useful to be able to run the coverage locally when needed. Otherwise, we'll try to get to this soon.

[mlms13]

@utkarshkukreti does the Windows build failure happen when Bucklescript tries to compile, or is it a postinstall failure when you npm install bisect_ppx? I'm wondering if it's enough for us to remove the dependency from bsconfig.json on publish, or if we need to remove it from package.json as well.

It looks like a script is available that can conditionally add (or maybe remove?) it during CI: https://github.com/wyze/conditional_bisect If possible, I think it would be good for us to find a CI solution like this so that we can avoid having to remember to edit files before a release (ideally even more of our release process could be pushed into CI). But maybe in the meantime we could just manually make a release without bisect.

My other concern is that our one dependency, Bastet, also uses bisect_ppx, so even if we manually remove it, you may still run into problems on Windows if Bastet requires it.

[utkarshkukreti]

@mlms13 I believe it's in the postinstall step (here's the log), so it needs to be removed from package.json too.

My other concern is that our one dependency, Bastet, also uses bisect_ppx, so even if we manually remove it, you may still run into problems on Windows if Bastet requires it.

Oh, I didn't consider this :(. Yeah, removing from Relude is not enough in that case, it needs to be removed from bs-bastet too.

[andywhite37]

Ugh, I didn't realize bastet was also using it. Yeah, maybe conditional_bisect is the way to go.

[utkarshkukreti]

@mlms13 @andywhite37 I haven't tested it, but I think moving bisect_ppx to a dev dependency and then using conditional_bisect in ci (or manually in dev) should work? I'll open a request on the bastet repo to do the same after I've tested that this works.

[andywhite37]

Cool, that works for me, thanks @utkarshkukreti

[utkarshkukreti]

So I tried this but since it's the npm install step that fails on Windows, moving it to dev dependencies still doesn't let you compile the package while developing on Windows.

I haven't used bisect_ppx, but according to the author of conditional_bisect, it's meant to be used only in CI. Will only running bisect on CI affect your workflow?

[utkarshkukreti]

Ok, just realized that he uses bisect-ppx-report send-to here.

[andywhite37]

I don't think coverage should be a CI-only workflow. I used to run the jest coverage locally all the time to see what needed coverage. Tracking coverage percentages via CI is kind of just a nice-to-have in my opinion.

Prior to bisect_ppx, we used Jest coverage, but it was not great because it ran coverage on the compiled JS code, rather than the actual OCaml/Reason code. bisect_ppx seemed like it would be better because it would run the instrumentation on the actual ocaml/reason code, but ppxs in general have proven again and again to be fragile and not cross-platform, and bisect_ppx is apparently no exception to that.

If it were me by myself, I would probably just switch back to jest coverage and declare the bisect_ppx effort a miss. It sucks that we have to jump through all these hoops and mess with dependencies just to run coverage on the code. Does anyone have any ideas? I'm not in favor of a CI-only workflow for running coverage, and it would be nice if the solution we go with doesn't involve a dependency on pre-compiled binaries that aren't cross-platform.

@utkarshkukreti - are you able to npm install using the --ignore-scripts flag, so it doesn't try to do the post-install stuff? I'm not sure what other side effects that has in the install - it probably fails to install bs-platform too 😢

[johnmegahan]

Somewhat related to this, I tried building in the 9.1.1 version of rescript, but seems a module bisect needs to compile has been removed recently.

FAILED: src/common/bisect_common-Bisect.cmj
File ".../node_modules/bisect_ppx/src/common/bisect_common.ml", line 184, characters 3-20:
Error: Unbound module Marshal
FAILED: cannot make progress due to previous errors.
Failure: .../node_modules/rescript/darwin/ninja.exe
Location: .../node_modules/bisect_ppx/lib/bs

rescript-lang/rescript@8a1fa05

[chrischen]

Somewhat related to this, I tried building in the 9.1.1 version of rescript, but seems a module bisect needs to compile has been removed recently.

FAILED: src/common/bisect_common-Bisect.cmj
File ".../node_modules/bisect_ppx/src/common/bisect_common.ml", line 184, characters 3-20:
Error: Unbound module Marshal
FAILED: cannot make progress due to previous errors.
Failure: .../node_modules/rescript/darwin/ninja.exe
Location: .../node_modules/bisect_ppx/lib/bs

rescript-lang/rescript-compiler@8a1fa05

Hongbo mentions why it's broken. They removed Marshal and so it doesn't work, though he claims Marshal never worked anyways so in theory this should be fixable.

https://forum.rescript-lang.org/t/some-thoughts-on-community-building/1474?u=notchris

Stability: For example, in the latest release, we removed the Marshal module, this module was never supported, any use of its API would cause a runtime crash, so in theory it should not break anything unless you had a crash before. But it does break some important packages, since it used a ppx which used Marshal, I have no idea why Marshal is needed.

[WhyThat]

Somewhat related to this, I tried building in the 9.1.1 version of rescript, but seems a module bisect needs to compile has been removed recently.

FAILED: src/common/bisect_common-Bisect.cmj
File ".../node_modules/bisect_ppx/src/common/bisect_common.ml", line 184, characters 3-20:
Error: Unbound module Marshal
FAILED: cannot make progress due to previous errors.
Failure: .../node_modules/rescript/darwin/ninja.exe
Location: .../node_modules/bisect_ppx/lib/bs

rescript-lang/rescript-compiler@8a1fa05

I tried to update to rescript@9.1 too, and I have the same error, my project is compiling when I remove things relative to bisect-ppx in the bs-config.
Do you think it's possible to have a prepublish script who remove these things from package.json and bs-config.json ? (we have to do this in bs-bastet also)

[andywhite37]

Sorry for the delayed action/response - thanks for submitting that PR! I haven't been following the saga of bisect_ppx and rescript - is there a chance it will ever work again? If not, I'm sort of inclined to just remove it completely and go back to the jest coverage we had before.

If there is a chance it might work again at some point, we can do the prepackage script - I'll try to checkout the PR soon

[WhyThat]

Don't really know, but in fact bisect_ppx is dependency only needed for development purpose, I think it's legit to remove it from the published package and keep it for development purpose imo

[andywhite37]

Yeah, I agree it is only for dev time - the issue has always been that reason/bucklescript/rescript didn't support ppxs as dev dependencies, so it always had to exist as a prod dependency. It's been left as a prod dependency b/c otherwise we'd have to do this package.json juggling like we're considering now, which I'd kind of like to avoid. I'd rather just have dependencies setup in a sane way in the package.json, and for things to just work without all this intervention, so that's why I'm inclined to just get rid of bisect_ppx and the complexity that comes with it. Jest is not great, but it kind of got the job done before, and seemed to just work.

[mlms13]

Has anyone chatted with @Risto-Stevcev about his interest in doing something like this in Bastet as well? I really liked the idea of bisect-ppx and I'd be a bit sad to lose it, but we were always hoping that Rescript would eventually add better support for (dev-time) PPXs, and instead it seems like things are moving in the opposite direction.

If things aren't going to improve on the Rescript side, I'd tend to agree with @andywhite37 that we're better off removing the dependency instead of fighting the direction of upstream.

[mlms13]

Alternatively... how is Melange's support for dev-only PPXs? (cc @anmonteiro) It might be time to accept the fact that Relude has never lined up with the Rescript vision, and we should embrace Melange instead. I already kind of want to do this for other reasons (let ops), but I was hoping we'd be able to make one final 1.0 release that still officially supports Rescript.

That said, Bucklescript/Rescript has been breaking Relude through poorly-communicated changes for years, and I'm running out of energy to guarantee that Relude will always be compatible with the Rescript compiler.

[chrischen]

It might be time to accept the fact that Relude has never lined up with the Rescript vision

I hope you guys keep supporting ReScript!

[anmonteiro]

Melange doesn't support dev-time PPXes right now, but it shouldn't be very hard to add it. I'll create an issue upstream and work on it soon.

[ghost]

Sorry for the delayed action/response - thanks for submitting that PR! I haven't been following the saga of bisect_ppx and rescript - is there a chance it will ever work again? If not, I'm sort of inclined to just remove it completely and go back to the jest coverage we had before.

If there is a chance it might work again at some point, we can do the prepackage script - I'll try to checkout the PR soon

Hopefully the relude family can upgrade to the latest bisect-ppx version, since aantron said he updated it to no longer reference the Marshal module?

[Risto-Stevcev]

@utkarshkukreti @WhyThat @mlms13 Hey sorry I'm late for this one, Github never showed the mention in my notifications at all for some reason until this last comment

I'm swamped this next week with work but I'm taking vactation after that, I can take a look at these chain then