Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document alternate build workflow using esy #1590

Merged
merged 3 commits into from
Jan 2, 2018

Conversation

zploskey
Copy link
Contributor

@zploskey zploskey commented Nov 2, 2017

Describe what is necessary to build the project with esy after the
opam instructions, with the caveat that it is still experimental.

@jordwalke
Copy link
Member

jordwalke commented Nov 3, 2017

Not sure I wanted to advertise this until it's stable. We might break some people who rely on this workflow while we move fast to complete the important esy features. I'll hold off on merging this for a little bit but you can feel free to help some other people try out the esy development workflow if you think they might be interested. I personally find it much more reliable than the opam workflow due to the sandboxed nature/lock files.

@zploskey
Copy link
Contributor Author

zploskey commented Nov 3, 2017

I also prefer this workflow, even if it might have breaking changes. I was hoping to have something to point people to, even if it doesn't get merged right away. Although I figure people building from source/contributors ought to be able to contend with it if they want to use an experimental feature. If you like, I'll try to keep this PR updated if there are breaking changes.

@jordwalke
Copy link
Member

Andrey just sent out a PR for the "fast build" workflow which is pretty slick, and was the major missing piece required before encouraging people to experiment with esy. If you can help us test/polish it then it will speed up the time it takes to feel comfortable pointing people to this workflow.

@zploskey
Copy link
Contributor Author

zploskey commented Nov 3, 2017

I can put some time into testing fastbuild next week.

@zploskey
Copy link
Contributor Author

zploskey commented Nov 10, 2017

On every version of esy since 0.28 (I tried 0.0.29-0.0.32), this series of commands fails:

$ esy install && esy build && esy make test
...
    ocamlopt src/refmt/refmt_impl.exe
rm -rf ./formatTest/**/actual_output
rm -f ./formatTest/failed_tests
rm -f ./miscTests/reactjs_jsx_ppx_tests/*.cm*
# I don't have modern enough node to test. brb.
node ./formatTest/testOprint.js
Done! 0 failures
./miscTests/rtopIntegrationTest.sh
Testing rtop...
rtop is failing! Failed to evaluate `let f = (a) => a;`
make: *** [Makefile:19: test] Error 1

On 0.0.28 it succeeds and all the tests pass.

@jordwalke
Copy link
Member

This is expected for now, and it's the one missing feature we need in esy before encouraging the esy workflow for contributing to the Reason repo.

Here's the missing feature: after you build a project, you can run commands like esy vim or esy sh -c 'echo $PATH', which run commands in an environment that is the build environment + your dev tools. It intentionally doesn't include the artifacts that you produce as part of the build, like rtop (the mistakenly did previously, and now they don't). The final feature we need before encouraging esy for developing the Reason repo, is a command like esx rtop which will see your own project's build artifacts as if it's installed.

@zploskey
Copy link
Contributor Author

I see. Until esx is usable (not sure if there is anything built yet), I've been able to get the tests to run by commenting out ./miscTests/rtopIntegrationTest.sh in the Makefile.

@jordwalke
Copy link
Member

It shouldn’t be too hard to implement esx. I don’t expect it to be too long. The more issues you can find with esy build in the meantime the more polished it will be when we are ready to have people try it out.

@jordwalke
Copy link
Member

You can give the workflow a try with npm install -g esy@next. The command to run things in a built project is esy x any-command-here. For example, in the Reason project, after running esy build, you could run esy x rtop to test the locally built rtop. Think of esy x as like a way to temporarily install a package to your machine without having to actually install it. It installs it for the duration of that command. If you want to give it a shot, and file any issues you find on the esy repo, that would be awesome.

@zploskey
Copy link
Contributor Author

I will try it out. Thanks for letting me know about it.

@zploskey
Copy link
Contributor Author

zploskey commented Nov 22, 2017

Doing exy x make test works for me in the reason repo at least.

Describe what is necessary to build the project with esy after the
opam instructions, with the caveat that it is still experimental.
src/README.md Outdated
The esy workflow is still experimental.
If you would like to help test, you can try it with the following commands.
If you already use OPAM for Reason, take care to deactivate OPAM first
or ensure your node binaries precede OPAM's in your path.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s the opposite. You’d want node binaries to come after opam’s. Also, esy does a good job of scrubbing the env during builds so I’m not sure this warning is necessary. Did you run into an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this actually may not be necessary, although so far I found that esy basically eliminated my need to use opam directly. Probably best to remove this warning.

Copy link
Member

@jordwalke jordwalke Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But your initial concern about people having opam already installed is well-founded - it happens a lot. I think we should be okay without this warning though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I think if you use esy ... prefix (esy x make test and so on) in workflow the fact that OPAM is installed and how it's configured is totally irrelevant. It's a bug in Esy in any other case.

@jordwalke
Copy link
Member

Thanks. I'll wait to land this when the new esy release happens (which will speed up esy x commands). In the mean time, please file bug reports in the esy repo for any issues you find.

Copy link
Member

@jordwalke jordwalke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jordwalke jordwalke merged commit f17d39a into reasonml:master Jan 2, 2018
@zploskey zploskey deleted the document_esy_workflow branch January 2, 2018 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants