-
Notifications
You must be signed in to change notification settings - Fork 17
Port from ocaml-migrate-parsetree to ppxlib #20
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
Conversation
src/ppx_blob.ml
Outdated
| let dirname = | ||
| let loc_fname = loc.Location.loc_start.pos_fname in | ||
| if Filename.is_relative loc_fname then | ||
| Filename.dirname (Filename.concat (Sys.getcwd ()) loc_fname) | ||
| else Filename.dirname loc_fname | ||
| in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremiedimino, is there a better replacement for Location.absolute_path that is exposed by Ppxlib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked in details, but Location.absolute_path might be a good candidate for addition in ppxlib. Could you open a ticket on github.com/ocaml-ppx/ppxlib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let dirname = | |
| let loc_fname = loc.Location.loc_start.pos_fname in | |
| if Filename.is_relative loc_fname then | |
| Filename.dirname (Filename.concat (Sys.getcwd ()) loc_fname) | |
| else Filename.dirname loc_fname | |
| in | |
| let dirname = Ocaml_common.Location.absolute_path loc.Ocaml_common.Location.loc_start.pos_fname |> Filename.dirname in |
Would using Ocaml_common temporarily solve the issue?
|
Thank your for your work on the migration. Is it supposed to supersede your other PR (#19)? One problem I've found is that with this new code, a file that's not found by ppx_blob leads to an uncaught exception ("Fatal error" with stack trace), whereas it used to trigger a regular compilation error. |
|
@bbc2, this could supercede #19, but this is a larger change and perhaps there are other uses which need a less abrupt migration, in which case #19 is maybe useful. In particular, it is a minimal change to unblock some uses of new features in code bases using ppx_blob, while this port is less pressing. I was not anticipating the change in exception behavior. Do you have a backtrace you could share here? |
|
Here's a backtrace: let () = print_string ([%blob "data.txt"])
|
|
Any news on this? This would be really nice to have with the upcoming OCaml 4.12 release, as OCaml 4.13 will probably not support ocaml-migrate-parsetree < 2.0 and this complicates package compatibility between opam packages otherwise. |
|
I fixed the uncaught exception issue and opened a PR on @jberdine's fork here: jberdine#1 |
|
Thank you all for all the help! I've rebased the commits and removed the intermediate merge commit. I've added CI tests to make it easier to detect potential issues. I'll merge and release a new version as soon as CI is green. |
CHANGES: Port to ppxlib (johnwhitington/ppx_blob#20).
resolve ocaml-ppx/ppxlib#139