various fixes #1585
various fixes #1585
Conversation
This will make them resilient against adding multiple opam packages to the repo. And also, it will use the number of jobs as set by OPAM.
There was a problem hiding this comment.
Looks good - will merge but the CI is failing because there's a few missed warnings due to being more strict. Why don't you just remove the --dev flag to jbuilder build so we can accept this, and then we'll later clean up the rest. (We could have a make build and make build-strict if you like).
| @@ -10,7 +10,7 @@ bug-reports: "https://github.com/facebook/reason/issues" | |||
| dev-repo: "git:https://github.com/facebook/reason.git" | |||
| tags: [ "syntax" ] | |||
| build: [ | |||
| ["jbuilder" "build"] | |||
| ["jbuilder" "build" "-p" name "-j" jobs] | |||
There was a problem hiding this comment.
Would you mind explaining what could break down if we were to add multiple packages to this repo?
Does this have jbuilder filter out any root directories that have their own opam files? (Preventing it from getting confused)?
There was a problem hiding this comment.
Sure. Suppose you have reason-rtop and reason opam packages in your repo. Here are some situations where your old build instructions are inadequate:
-
A user doing
$ opam install reasonwill build the code forreason-rtopas well. Which might fail because not all the deps might be installed by opam yet. -
A user doing
$ opam install reason-rtopwill be have rtop be built against thereasonfrom its local source tree and not the one that is already installed by opam. This is wrong in cases of pins, users mixing and matching versions (which I don't suggest that you support. But you shouldn't break it out right).
| @@ -5,7 +5,7 @@ SHELL=bash -o pipefail | |||
| default: build | |||
|
|
|||
| build: | |||
| jbuilder build | |||
| jbuilder build --dev | |||
There was a problem hiding this comment.
Do you find this slows down builds?
There was a problem hiding this comment.
It probably does. Checking for warnings isn't free. I do think the overhead is usually negligible though.
| @@ -483,7 +483,7 @@ and print_typlist print_elem sep ppf = | |||
| print_typlist print_elem sep ppf tyl | |||
| and print_out_wrap_type ppf = | |||
| function | |||
| | (Otyp_constr (id, _::_)) as ty -> | |||
There was a problem hiding this comment.
I actually don't mind unused variables so much - they're like documentation. I'll merge this though because we encourage _ for unused variables in Reason React apps too.
There was a problem hiding this comment.
In the case you can simply prefix the vars with _ and there won't be a warning. I only replaced the 1 letter and other useless sounding names with _. Others I've prefixed.
| @@ -441,7 +441,7 @@ module Help = struct | |||
| | `M_main -> str "$(b,%s) $(i,COMMAND) ..." (invocation ei) | |||
| | `Simple | `M_choice -> | |||
| let rev_cmp (p, _) (p', _) = match p', p with (* best effort. *) | |||
| | p, All -> -1 | All, p -> 1 | |||
There was a problem hiding this comment.
This one is vendored so we should probably just use whatever the original author had (and we can update the vendored library with their latest version). Still, merging because we'll likely overwrite this with their latest version.
There was a problem hiding this comment.
Yes, you should update cmdliner. I'm pretty sure Daniel fixed these warnings.
I did some little clean ups that you can cherry pick as you wish. The first commit isn't really optional though.
Holy mother of god with regards to those warnings. You all need to clean house here. I definitely don't have time for all these.