search

revery-ui / reason-reactify

:rocket: Transform a mutable tree into a functional React-like API
MIT License
103 stars 8 forks source link

Remove buildsInSource #32

Closed jchavarri closed 5 years ago

jchavarri commented 5 years ago

This is more of a question than anything else 😄

I've noticed this repo and others use buildsInSource in the esy config. I wonder why this is needed? Is it for CI, or better Windows support?

What drove me to ask is that this configuration breaks reason-language-server –in macOS at least–, see https://github.com/jaredly/reason-language-server/issues/209. So I wondered about whether it could be removed.

If not, I can always keep working with this option disabled while developing, until a fix in RLS lands. :)

jchavarri commented 5 years ago

Ooh, I guess it is because of Windows & symlinks? 🤔

bryphe commented 5 years ago

Ah good question! There were two reasons I was using this setting:

It looks like the build is failing here when it tries to run the node tests against the output test.bc.js - running the esy build:js and esy test:js commands. We might have to create a wrapper node script that runs esy which Test.bc.js to get the actual path, instead of hardcoding it in the package.json: https://github.com/bryphe/reason-reactify/blob/dfc95a79d8e190bd9bfac4b388958f3b5cb3ef1f/package.json#L10

jchavarri commented 5 years ago

Thanks for the feedback!

It looks like the build is failing here when it tries to run the node tests against the output test.bc.js - running the esy build:js and esy test:js commands. We might have to create a wrapper node script that runs esy which Test.bc.js to get the actual path, instead of hardcoding it in the package.json:

So, this should be in theory pretty straight forward, just by leverating esy which as you mention. But there are two issues:

  1. which doesn't work with regular files, just executables
  2. I tried to create a Test.exe binary and then using esy x which Test and esy which Test (without the x) but I always get /bin/test back, which seems like a bug? Also, there is no way I can run this native version with esy x Test, but if I get a handle to the binary path, it works 🤔

I'm not sure if 2 is a problem with Dune? Or does esy treat executables with "Test" in them specially? cc @andreypopp

jchavarri commented 5 years ago

Another strange thing is that if one pulls this branch and runs:

rm -rf _esy
rm -rf _build
esy install
esy build
esy which Test

The output is /bin/Test. (in esy 0.4.4-e128aa)

bryphe commented 5 years ago

Hey @jchavarri ! Thank you for looking at this!

The output is /bin/Test. (in esy 0.4.4-e128aa)

Ah, I think there is a name collision - it turns out there is a test binary that is in /bin or /usr/bin that bash uses for evaluating expressions. So it's a poorly named executable! I think simply renaming it to something like TestRunner will be the easiest thing.

which doesn't work with regular files, just executables

Hmm, yes, this makes it hard to find the output of Test.bc.js (or TestRunner.bc.js now). I'm not sure the best way to locate this? I guess if we can find TestRunner.exe - the TestRunner.bc.js should be in the same place.

Even after renaming it - I don't see it via esy b which TestRunner.exe - I think this is because we don't actually install it. So really only 'dune' knows where it's at.

I imagine there's something we could do in the dune rules to run it. I tried creating a custom alias to run the tests:

(alias
    (name runtestjs)
    (deps ./test/TestRunner.bc.js)
    (action (run node ./test/TestRunner.bc.js)))

Actually - one thing I tried just worked. Updated the test:js to:

    "test:js": "node #{self.target_dir}/default/test/TestRunner.bc.js"

I think this combination of fixes should work:

🤞

andreypopp commented 5 years ago

Ah, I think there is a name collision - it turns out there is a test binary that is in /bin or /usr/bin that bash uses for evaluating expressions. So it's a poorly named executable! I think simply renaming it to something like TestRunner will be the easiest thing.

Hm... I think esy should resolve Test from reason-reactify before /usr/bin/test. The reason it doesn't do that now is because Test isn't being installed (lacks (public_name ...) in its stanza). The alternative would be to use dune exec if we don't want to install test binaries (and I'm not sure we want):

% esy dune exec test/Test.exe

(maybe I messed dune invocation above but it should be something like this)

andreypopp commented 5 years ago

which doesn't work with regular files, just executables

In nighties we can refer to variables from the command line like this:

% esy ls '#{self.install}'
% esy ls '#{self.install}/share'

It's a bit verbose but allows to peek into installation directory.

jchavarri commented 5 years ago

Thanks for all the info! I managed to fix it by just using self.target_dir as @bryphe mentioned 🙂 All green now! ✅

Rename Test -> TestRunner

I didn't see the need to rename Test to TestRunner, but I could do it if required.

(side note: I'm so surprised the Windows build takes 25min+ and Linux and macOS only 5/6min? 😮 ) (side note 2: wasn't there a way to cache esy builds too, to make CI faster?)

bryphe commented 5 years ago

@jchavarri - looks great! Thank you for this!

I didn't see the need to rename Test to TestRunner, but I could do it if required.

Doesn't seem necessary now - as long as the builds are green, I'm happy 😄

(side note: I'm so surprised the Windows build takes 25min+ and Linux and macOS only 5/6min? 😮 )

Yes, unfortunately the OCaml compiler takes a loooong time to build on Windows.

(side note 2: wasn't there a way to cache esy builds too, to make CI faster?)

Yes! @ksuperplane did some great work on improving the cache - need to port over the changes here: https://github.com/esy-ocaml/esy-ci-scripts/pull/6 I'll bring that over shortly.