reason icon indicating copy to clipboard operation
reason copied to clipboard

[internal] get the reason bspack build script working with esy

Open jaredly opened this issue 7 years ago • 33 comments

This basically means all you need is to clone the repo and do cd bspacks; ./bspack.js and it will All Work™

I think this is a much smoother workflow... lmk if there's anything I haven't thought of though.

(this is motivated in part by the desire to make reason releases easier to do)

jaredly avatar Aug 21 '18 06:08 jaredly

Ready to have this merged?

jordwalke avatar Aug 23 '18 06:08 jordwalke

I think I have a different sed:

sed: can't read s/"ocaml": "~4.6.0"/"ocaml": "~4.2.3004"/: No such file or directory

I recall installing gnu-sed because the macOS sed is old. What to do here? This might fail CI

chenglou avatar Aug 23 '18 07:08 chenglou

When using -i on my mac I had to use '' but that fails on Linux where it's not needed and I think that is what @chenglou is hitting.

ulrikstrid avatar Aug 23 '18 11:08 ulrikstrid

@jaredly hold on, you make refmt work with bsc. Maybe the sed can be dropped altogether?

chenglou avatar Aug 24 '18 02:08 chenglou

@chenglou I changed set to use gnu format. And the "make it work with bsc" is a rather more invasive change, I wanted to get this merged first.

jaredly avatar Aug 24 '18 22:08 jaredly

On newest Esy:

❯ version=3.4.5 ./reason_bspack.sh
info esy install 0.2.6
info esy build 0.2.6
info found esy manifests: esy.json
esy: error, exiting...
     invalid dependency @opam/ocaml: unable to resolve package
       processing package: get-js-of-ocaml@xxx
       processing package: @opam/base-ocamlbuild@base

chenglou avatar Aug 25 '18 00:08 chenglou

@chenglou latest is 0.2.7 -- could you try that?

jaredly avatar Aug 25 '18 03:08 jaredly

@chenglou your error says there's a problem finding @opam/ocaml, but nothing should be depending on that (the package doesn't exist). Did you modify the package.json before testing? Maybe you forgot to revert your local tweak. (It should always be ocaml not @opam/ocaml because we host the compilers on npm).

jordwalke avatar Aug 25 '18 06:08 jordwalke

This looks like some opam 2 metadata got in the sandbox (opam 2 models OCaml as a dependency and thus we see @opam/OCaml). This warrants trying with esy@>0.2.6 which can handle opam 2 metadata. On Sat, 25 Aug 2018 at 09:41 Jordan W [email protected] wrote:

@chenglou https://github.com/chenglou your error says there's a problem finding @opam/ocaml, but nothing should be depending on that (the package doesn't exist). Did you modify the package.json before testing? Maybe you forgot to revert your local tweak. (It should always be ocaml not @opam/ocaml because we host the compilers on npm).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/facebook/reason/pull/2153#issuecomment-415944837, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB3glNRJyctyvOSgNLHJsT1_mA1-fMKks5uUPGMgaJpZM4WFJ3H .

andreypopp avatar Aug 25 '18 07:08 andreypopp

Explanation on how opam 2 metadata got into the sandbox: esy.lock.json was produced with [email protected] which upgrades opam 1 to opam 2 automatically and stores opam metadata in the lockfile (we do that so lockfile could lock state of the opam repo at the time of creation) so [email protected] sees opam 2 metadata and fails. There's an issue for esy — we need to specify minimal esy version in a lockfile.

andreypopp avatar Aug 25 '18 16:08 andreypopp

As long as these tests run on the latest I think this should be good to merge?

jordwalke avatar Aug 25 '18 22:08 jordwalke

The tests don’t run on latest yet

chenglou avatar Aug 25 '18 22:08 chenglou

@chenglou what do you mean? What errors are you getting?

jaredly avatar Aug 25 '18 23:08 jaredly

[email protected] was released under next, not latest. I've just made [email protected] to be latest so it should work with latest now.

andreypopp avatar Aug 26 '18 08:08 andreypopp

I think this should no longer be an error since no one will be installing 2.7 from now on. This good to merge? This makes it much easier to test the bspacked form of reason since we don't have to use opam.

jordwalke avatar Aug 27 '18 05:08 jordwalke

Lemme year again when I get a laptop. I don’t know why my attempts keep failing

chenglou avatar Aug 27 '18 06:08 chenglou

Back. I now get this error:

❯ esy --version
0.2.11
❯ version=3.4.5 ./reason_bspack.sh 
esy: invalid semver version: 'xxx'

chenglou avatar Sep 08 '18 00:09 chenglou

Ok, fixed it up. Now works with esy 0.2.11

jaredly avatar Sep 12 '18 04:09 jaredly

I'd love to get this checked in - also to serve as an example of how to use bspack on arbitrary projects with arbitrary dependencies. Did you intend on adding bspack.ml to this? Isn't it generated? Let me know if it's ready to merge.

jordwalke avatar Sep 14 '18 06:09 jordwalke

I'll test it later. What's the hurry? It's not like this part is every run except by myself

chenglou avatar Sep 14 '18 07:09 chenglou

I occasionally test this just to make sure I didn't break anything. It was always such a pain because it involved opam and a bunch of other things. No rush though, fi you'd like to hold off.

jordwalke avatar Sep 14 '18 07:09 jordwalke

Ah ok, thanks. I'll do it later

chenglou avatar Sep 14 '18 07:09 chenglou

@jordwalke I did intend to add bspack.ml, and it is generated -- I vendored it here so that you wouldn't have to have the bucklescript source locally

jaredly avatar Sep 25 '18 16:09 jaredly

(from my side this is ready to merge)

jaredly avatar Sep 25 '18 16:09 jaredly

Trying this again. I got:

File "reason_parser.ml", line 5, characters 2-42:
Error: Unbound value MenhirLib.StaticVersion.require_20180905

chenglou avatar Oct 05 '18 07:10 chenglou

That means it's finding the wrong version of menhir :/ I'll take a look.

jaredly avatar Oct 05 '18 14:10 jaredly

I picked up from @jaredly's branch, and updated with latest master: https://github.com/jchavarri/reason/tree/joo. Feel free to incorporate it to this PR if it's useful.

Most relevant changes:

  • Move to esy.lock folder format of recents versions of esy
  • Added ppx_derivers as explicit dev of bspacks, as it's been added as dep of omp recently
  • Make sure bspacks uses the same version of menhir as the parser. I ran into the problem about StaticVersion mentioned above but it seems it happens when there are stale builds of the parser locally. I managed to solve it by cleaning build artifacts from parser and building everything again (so that both parser and bspacks use the same version).

I am blocked on one more error though, it seems the new build config for reason_parser.mly doesn't produce the ml file on build folder, so I get this:

File "src/reason-parser/reason_declarative_lexer.mll", line 70, characters 5-18:
Error: Unbound module Reason_parser

What'd be the change needed to produce a reason_parser.ml file from the menhir file?

jchavarri avatar Nov 25 '19 00:11 jchavarri

Managed to fix the menhir issue, there were more paths that had to be migrated from _build to _esy.

So both refmt binary and refmt js api seem to be building ok now 🎉 Fixed some issues with omp modules being generated by bspack like Migrate_parsetree__Ast_404 (picked the fix from https://github.com/facebook/reason/pull/2495/, thanks @ryyppy!)

Also, updated refmt js api to the latest breaking changes after reerror.

Same branch: https://github.com/jchavarri/reason/tree/joo

jchavarri avatar Nov 26 '19 00:11 jchavarri

@jaredly Added another small change so that the script gets bspack.ml from BS github repo: https://github.com/jaredly/reason/pull/3.

Some future tasks could be:

  • migration to 4.06 for bs7
  • publish refmt-js-api to npm (i think it's reason? very confusing...) directly from GH actions, whenever there's a new release or merge to master
  • maybe publish bspacked-refmt to npm with same version using GH actions as well, so it can be consumed from BuckleScript's esy.json that @ulrikstrid has been working on.

As far as I can tell though, this is ready to merge.

jchavarri avatar Nov 28 '19 23:11 jchavarri

@jchavarri ok I added sandboxes, so now this script works with both 4.02 and 4.06! Very clean

jaredly avatar Dec 02 '19 18:12 jaredly

I opened this PR against bucklescript which should allow us to use bspack from upstream instead of vendoring it.

ulrikstrid avatar Dec 04 '19 15:12 ulrikstrid

Ok @jordwalke @ulrikstrid @jchavarri I think this is good to go!

jaredly avatar Dec 12 '19 06:12 jaredly

What is stopping this from going in? Seems like something that would be nice since the community wants to upstream reason changes into bucklescript more frequently

ulrikstrid avatar Apr 06 '20 17:04 ulrikstrid