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

[WIP] Fill in dune-project file #116

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

smolck
Copy link

@smolck smolck commented Dec 31, 2019

A few things need to be filled in before this is ready for merge:

  • The description part of the (package section.
  • The depends part of the (package section. I'm not sure what version of OCaml should used, so I've left it as (ocaml (>= <FILL VERSION HERE>)). If anyone knows the version that should be used, let me know and I'll update this. Also, any other dependencies will need to be put here, which don't necessarily have to have versions associated with them but should be listed.

If I'm forgetting any other fields (or if there are any other changes that should be made), let me know. Note that I've used https://github.com/rgrinberg/opium/blob/master/dune-project as a reference here.

The reason for this addition is so that Revery can be published on OPAM (see revery-ui/revery#71), which will require that its dependencies be published on OPAM as well (this repository was mentioned as a dependency of Revery in revery-ui/revery#71 (comment)). With this PR, the dune-project file will now generate reglfw.opam (due to (generate_opam_files true)), preparing this library for publishing on OPAM.

dune-project Outdated
(package
(name reglfw)
(synopsis "Cross-platform Reason / OCaml bindings for GLFW")
(description "")
Copy link
Author

Choose a reason for hiding this comment

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

Need a description here.

Copy link
Member

Choose a reason for hiding this comment

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

Same as synopsis is good.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I've pushed your changes. It would be nice to have a more detailed description though if possible.

dune-project Outdated Show resolved Hide resolved

(source (github revery-ui/reason-glfw))
(authors "Bryan Phelps")
(maintainers "Bryan Phelps <bryphe@outlook.com>")
Copy link
Author

Choose a reason for hiding this comment

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

Should anybody else be added to these authors and maintainers fields?

@Et7f3
Copy link
Member

Et7f3 commented Dec 31, 2019

You haven't run opam lint :) because the lint say we must add a . at the end of synopsys and description. And sorry I hadn't seen the message is outdated. I have added a notice now that is up to date. Revery is know to compile from 4.07 to 4.09 maybe he compile on 4.06 (so just try to compile with 4.06 if you want. If you want to quickly move on you can put this three version.)
Also side note.
We have 3 types of packages
(1) reason-smth that are often binding to (2)esy-smth on npm or (2)conf-smth in opam world so we have a depext to smth.

(3) We have also regular deps that are purely OCaml/ReasonML. some with @opam/ preffix are already on opam so it is less work to do :).

Reason-smth has of course dep to reason (published on OPAM)

@Et7f3
Copy link
Member

Et7f3 commented Dec 31, 2019

But seem to be fine just need to do that for other package.

@smolck
Copy link
Author

smolck commented Dec 31, 2019

You haven't run opam lint :) because the lint say we must add a . at the end of synopsys and description.

Sorry about that, should work now.

And sorry I hadn't seen the message is outdated. I have added a notice now that is up to date. Revery is know to compile from 4.07 to 4.09 maybe he compile on 4.06 (so just try to compile with 4.06 if you want. If you want to quickly move on you can put this three version.)

I've put the version to >= 4.07 if that's alright with you? (Note that it seems to work with 4.06 so I can change the version to that if you want.)

@smolck
Copy link
Author

smolck commented Dec 31, 2019

Is there a dependency we're missing in the dune-project file that's causing the Linux build to fail? It seems it "cannot find -lGL" and "cannot find -lGLU": https://dev.azure.com/revery-ui/revery/_build/results?buildId=3681&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=6c939d89-0d1a-51f2-8b30-091a7a82e98c&l=147.

Apparently the MacOS build failed due to a permissions error, not sure why that happened: https://dev.azure.com/revery-ui/revery/_build/results?buildId=3681&view=logs&j=a5e52b91-c83f-5429-4a68-c246fc63a4f7&t=2b3990e0-410c-52b9-0fec-7c266a5d9fcd&l=14

@Et7f3
Copy link
Member

Et7f3 commented Feb 6, 2020

We install it in ci before running esy https://github.com/revery-ui/reason-glfw/blob/master/azure-pipelines.yml#L21

@Et7f3
Copy link
Member

Et7f3 commented Feb 6, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Et7f3
Copy link
Member

Et7f3 commented Feb 6, 2020

mac os with error 127 are back 🤔 and pull rejest. I don't know what happen. For the linux issue it seem the dune file are out of date (and seem to ignore config folder) https://github.com/revery-ui/reason-glfw/blob/master/config/discover.ml#L43-L44

@smolck
Copy link
Author

smolck commented Feb 7, 2020

Hmm, so maybe we use a later version of dune then? I’m not sure about getting dune to recognize the config, I’d have to look into that.

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.

2 participants