ppx_cstruct 4.04 #111
ppx_cstruct 4.04 #111
Conversation
| @@ -4,7 +4,7 @@ V=`ocamlc -version` | |||
| case $V in | |||
| 4.02*) SRC=402 ;; | |||
| 4.03*) SRC=403 ;; | |||
| *) echo Unsupported OCaml version $V ;; | |||
| *) SRC=error; echo Unsupported OCaml version $V ;; | |||
There was a problem hiding this comment.
isn't an exit 1 sufficient, rather than distributing invalid ml code for the error case?
There was a problem hiding this comment.
Indeed it works fine, I'll rebase the PR. Thanks @hannesm!
Before this commit, compiling ppx_cstruct from an unsupported OCaml
version would just build ppx_cstruct.native from an empty source file,
which worked but produced an incorrect preprocessor, deferring
failures to cstruct's users.
After this commit, using an unsupported OCaml version fails at
configure time:
```
Unsupported OCaml version 4.04.0+dev7-2015-08-09
E: Failure("Command 'sh select_ppx_version.sh conf' terminated with error code 1")
Makefile:34: recipe for target 'setup.data' failed
```
Note that ppx_cstruct.404.ml is just an unchanged copy of ppx_cstruct.403.ml. This seems to work well with the testsuite provided. I used a plain file copy instead of a symlink as I don't know how git symlinks are handled on Windows machines. Having two files that are exactly the same means a fair amount of redundancy, but I think this redundancy was chosen by the decision to have a ppx_cstruct.$VERSION.ml file per OCaml version in the first place. If one wants to reduce redundancy in the future, I guess that a single file with preprocessor directives would make more sense.
gasche
force-pushed
the
ppx_cstruct_4.04
branch
from
189c652
to
f8cd4ba
Compare
| V=`ocamlc -version` | ||
| case $V in | ||
| 4.02*) SRC=402 ;; | ||
| 4.03*) SRC=403 ;; | ||
| *) echo Unsupported OCaml version $V ;; | ||
| 4.04*) SRC=404 ;; |
There was a problem hiding this comment.
instead of duplicating code in the repo, we could set SRC=403 here.. I'm still not sure which is less worse - pest or cholera
There was a problem hiding this comment.
I considered it, but I think that it's better to be clear (and dumb) rather than try to be cunning inside a general approach that is rather duplication-friendly anyway.
|
thx @gasche. IMHO good to merge! |
|
ok, will cut a release shortly. |
| @@ -1,10 +1,17 @@ | |||
| #!/bin/sh -e | |||
|
|
|||
| case $1 in | |||
| clean) rm -f ppx/ppx_cstruct.ml; exit 0 ;; | |||
| conf) (* continue *) ;; | |||
There was a problem hiding this comment.
looking at the patch again, this line uses OCaml comment syntax, which provokes the following behaviour from the root of the repository:
$ (* continue *)
bash: appveyor.yml: command not found
(The first * expands to the list of files in alphabetic order and the first, appveyor.ml, gets called as a command)
It would be nice to commit a minor change to remove the comment or use proper shell comment syntax.
This PR implements support for 4.04 by just copying the 4.03 implementation which seems to work unchanged -- the local testsuite passes. This is an attempt to fix #110.
The first two commit make small changes on the
select_ppx_versionhack (see commit messages):distclean.It would be nice if a release with this PR or a similar change to bring 4.04 could be made before the actual 4.04 release, so that we can test (and possibly fix) other software depending on
cstructduring the beta/release-candidate period.