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

Port to jbuilder #30

Open
wants to merge 9 commits into
base: RudiPlayground
Choose a base branch
from
Open

Conversation

rgrinberg
Copy link
Contributor

@rgrinberg rgrinberg commented May 13, 2017

This PR requires a minimum version of jbuilder: 1.0+beta9. This is the first version when reason was introduced.

@jordwalke
Copy link
Member

jordwalke commented May 13, 2017

Wow thanks for providing an example so I can get a sense for how to use jBuilder. I don't have a lot of experience setting up native opam projects, so this is helpful to see.

  • I think you mis-named the opam file (it should say reason-native not react-native).
  • How does the README change? Does anything else get simpler?

In addition, I think what people would really want to see in a follow up diff:

  1. How you depend on another opam package. Supposed you wanted to depend on @bordoley's Immutable opam package. Maybe in a follow up diff you could show what that would look like?
  2. How you would export a consumable library, and a binary.

I can create a playground branch if you want to push your work to it. We probably wouldn't want to encourage using jBuilder until it stabilizes, but a playground branch would be an easy way for you to try things out while jBuilder stabilizes.

@rgrinberg
Copy link
Contributor Author

Sounds reasonable.

@jordwalke jordwalke changed the base branch from master to RudiPlayground May 13, 2017 02:53
* Using external dep
* Internal library
* Library for opam export
* Binary
@jordwalke
Copy link
Member

The pin is no longer necessary! Might want to update and we can start pointing people to this PR so they can test it out and get some feedback. Thanks again, @rgrinberg !

@jordwalke
Copy link
Member

@rgrinberg It would also be really great if we could use this as an opportunity to teach people how to use opam effectively for local projects. For example, would you be able to suggest a workflow to teach newcomers:

  • How to create a switch just for this project.
  • How to install just the dependencies for the project into the switch in one command.

I notice people mess these steps up frequently - especially when they have existing packages already installed in the global switch, which conflict.

@gilbert
Copy link

gilbert commented May 22, 2017

Bug report: When I clone down this PR and run make build, I get the following error:

Executable test in _build/default/reason-native-project doesn't have a corresponding .ml file

It seems it is overlooking the .re within that folder.

@jordwalke
Copy link
Member

@gilbert Which version of jbuilder are you using? I hear the latest does not require you to pin that git URL - you merely need to install beta v9.

@gilbert
Copy link

gilbert commented May 23, 2017

@jordwalke I thought I was using the latest, but it turns out I wasn't. Using jbuilder 1.0+beta9 worked, thank you!

@kennetpostigo
Copy link
Contributor

Hopefully this gets merged soon!

@jaredly jaredly mentioned this pull request Jun 13, 2017
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.

4 participants