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

Support for esy local development. #45

Merged
merged 1 commit into from
Jul 24, 2018
Merged

Conversation

jordwalke
Copy link
Contributor

Summary: This diff adds support for local development of the graphQL ppx
via esy. I added instructions about usage in the new DEVELOPING.md doc
and I hope some more info can be added there about how the CI system
works etc.

This doesn't implement all the CI and packaging using esy which will
eventually become possible once we get deep Windows support, but it
makes the local install/build iteration more enjoyable.

I also moved to the officially supported Dune approach for configuring
builds (discover.ml) which is what the Dune devs recommend. It also
was required for the local esy builds because esy doesn't want you to
write artifacts to your source tree (when your esy.json file
says it only writes to the _build directory).

Test Plan:Tried this out with esy and it works.

Summary: This diff adds support for local development of the graphQL ppx
via esy. I added instructions about usage in the new `DEVELOPING.md` doc
and I hope some more info can be added there about how the CI system
works etc.

This doesn't implement all the CI and packaging using esy which will
eventually become possible once we get deep Windows support, but it
makes the local install/build iteration more enjoyable.

I also moved to the officially supported Dune approach for configuring
builds (`discover.ml`) which is what the Dune devs recommend. It also
was required for the local esy builds because `esy` doesn't want you to
write artifacts to your source tree (when your `esy.json` file
says it only writes to the `_build` directory).

Test Plan:Tried this out with `esy` and it works.
let (l, _) = process_output_of command in String.trim (String.concat "\n" l)


let output = output_text "uname -s"

Choose a reason for hiding this comment

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

Don't do this as you will break cross compilation. Use ocaml_config_var_exn for such environment tests. See my example here: https://github.com/savonet/ocaml-ssl/pull/39/files#diff-81f275ec1ff7428a19d7cd928fb69c94

Also, use the configurator entry point function: C.main.

@mhallin
Copy link
Owner

mhallin commented Jul 21, 2018

It looks like Esy and npm/yarn are fighting against each other about what should be in the node_modules folder:

$ esy install
...
info installing: done

$ esy build
# Completes successfully 

$ esy test
dune build @graphql_ppx
cp _build/default/src/graphql_ppx.exe .
(cd tests && \
		../node_modules/.bin/bsb -clean-world && \
		../node_modules/.bin/bsb -make-world)
/bin/sh: ../node_modules/.bin/bsb: No such file or directory
make: *** [test] Error 127

# Fails, as expected - the node dependencies have not been installed yet

$ IS_GRAPHQL_PPX_CI=true npm install
# Completes successfully

$ esy test
esy: error, exiting...
     invalid dependency ocaml: unable to resolve package
       processing package: graphql_ppx@0.0.4
       processing package: @opam/conf-m4@1

# Fails because npm install removed the OCaml stuff

$ esy install && esy build
# Completes successfully

$ esy test
...
/bin/sh: ../node_modules/.bin/bsb: No such file or directory
make: *** [test] Error 127

# Fails because esy install removed the npm stuff

@jordwalke
Copy link
Contributor Author

I think that's because I didn't include bs-platform in the esy dependencies. I can try adding it there to see if it works, but the esy install and npm install aren't ever really supposed to happen with the same node modules. Esy just provides a better npm style workflow for building opam packages with opam dependencies. Then you can publish the result to npm without having esy be involved. I wanted esy support for this package because it makes my life way easier with the esy editor integration and install / build process during development. I would run the npm tests on pure npm for now.

@mhallin mhallin merged commit e8832fc into mhallin:master Jul 24, 2018
@mhallin
Copy link
Owner

mhallin commented Jul 24, 2018

I incorporated @rgrinberg's comments on discover.ml and updated the DEVELOPING.md file with opam instructions.

ylecornec pushed a commit to o1-labs/graphql_ppx that referenced this pull request Jun 3, 2022
Remove polymorphic comparison on Js.Json.t
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants