Fix flags for BuckleScript packages so that ppxs binaries can be found

[jchavarri]

There seemed to be an issue where MerlinFile.getFlags (used exclusively for BuckleScript packages as far as I can tell) was indirectly calling fixPpx which appears to be a fix for just bs-native, not regular bs.

This resulted in RLS erroring where a regular ppx binary foo was being called with lib/bs/native/foo.native which didn't exist.

Also, another issue was that BuckleScript runs ppxs with node-module/.bin in path, which breaks when RLS runs bsc manually just picking the name of the file from merlin.

This PR:

• Removes the call to fixPpx from MerlinFile.parseMerlin as it's not needed for BuckleScript
• Renames fixPpx to fixPpxBsNative for clarity to avoid potential issues like the one above
• Adds a new makePathForPpxBs that prefixes the ppx binary paths with node_modules/.bin.

As a side note, I found this because we migrated bs-emotion-ppx from bsb-native to esy, and now the binary name is just bs-emotion-ppx (which is added to "bin" field in package.json). I'm not sure if other ppxs would be in the same situation.

[jchavarri]

@jaredly I replaced the bs3.1.5 example (which I assumed could be discarded as it's a quite old version) with another with bs7 that shows the issue with ppx. To repro the problem:

• comment this line (|> List.map(makePathForPpxBs(rootPath)))
• reload the lang server
• reopen the bs7.0.1 example

[jchavarri]

Pasting from the convo in Discord:

If ppx-flags contains a slash like:

"ppx-flags": ["aa/bb"]

bsb will generate full path in merlin

FLG -ppx /Users/javi/Development/github/reason-language-server/examples/bs-7.0.1/node_modules/aa/bb

but if it's just a string without slashes:

"ppx-flags": ["bb"],

it generates:

FLG -ppx bb

From https://bucklescript.github.io/bucklescript/docson/#build-schema.json:

ppx-flags
PPX macros to pass to compiler. The syntax is package_name/binary, for example: reason/reactjs_jsx_ppx_3.native. Currenly searches in node_modules

---

So after understanding that bsb expects the format to be package_name/binary I don't think rls should add anything. I removed that code, but left the example and removal of bsb-native fix, which should not apply in that function, just in case it's useful :) sorry for the noise!

[jaredly]

ok, makes sense :)

[jaredly]

@jchavarri looks like the 7.0.1 example is failing on windows

[jchavarri]

@jaredly hm... the ppx definitely works on windows, we test it with a similar test project as the one here, see this build, windows job, step [bs-emotion-ppx/test] yarn build.

It seems the ppx can't find the input ast binary file in temp folder, I wonder if this has to do with the rls tests themselves? I originally thought it could be related to Azure setup , but I see how Appveyor ci fails too. Any ideas are more than welcome :)

[jchavarri]

I managed to run the failing command locally in a Windows machine. It works. I checked the command with bsb -verbose and can't see any appearing difference between them:

# In local machine (success)
c:\Users\IEUser\Downloads\bs-7.0.1\node_modules\@ahrefs\bs-emotion-ppx\bin\bs-emotion-ppx
"C:\Users\IEUser\AppData\Local\Temp\camlppx7cd714"
"C:\Users\IEUser\AppData\Local\Temp\camlppxb65b00"

# In CI machine (Appveyor, failed)
C:\projects\reason-language-server\examples\bs-7.0.1\node_modules\@ahrefs\bs-emotion-ppx\bin\bs-emotion-ppx
"C:\Users\appveyor\AppData\Local\Temp\1\camlppx824bf8"
"C:\Users\appveyor\AppData\Local\Temp\1\camlppx07d31e"

# In CI machine (Azure, failed)
d:\a\1\s\examples\bs-7.0.1\node_modules\@ahrefs\bs-emotion-ppx\bin\bs-emotion-ppx
"C:\Users\VSSADM~1\AppData\Local\Temp\camlppx70408b"
"C:\Users\VSSADM~1\AppData\Local\Temp\camlppxf275e9"

[jchavarri]

hm, ExamplesTests was running in bash mode which could explain why a path formed with C:\foo\bar fails to be found.

I'm trying to change that so that Azure runs ExamplesTests using yaml script which uses cmd.exe on Windows.

@jaredly It'd be really helpful to get the Azure cache PR reviewed and hopefully merged. It's very slow to debug this Windows stuff, as each build is taking 23+min 🐢 :)

[jchavarri]

ExamplesTests was running in bash mode which could explain why a path formed with C:\foo\bar fails to be found.

Narrator: it wasn't that.

[jchavarri]

@jaredly I rolled back the changes due to new example with ppx, as I couldn't figure out why the builds were failing on Windows. I tried node 8 and node 12, 32 and 64 bits, yarn and npm, windows 2017 and 2019, with and without esy, etc.

In the end, I managed to make test fail in both Appveyor and Azure by just npm run build in the bs project with a ppx. Unfortunately, the exact same commit would pass test some times and some other times failed. There seems to be something wrong with bsb and the way the ppx are called, that when the ppx binary gets called, the input ast file is not available anymore. I managed to get a couple of test passing using yarn, and node 12 (one, two).

The most confusing thing is that we're running similar tests in bs-emotion repo and I haven't seen anything like this. Maybe the changes in #361 will help?

[jaredly]

Ok, it's passing now, so I guess we're good to go!