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

Add support for camlp4 native plugin #5652

Closed
vicuna opened this issue Jun 18, 2012 · 18 comments
Closed

Add support for camlp4 native plugin #5652

vicuna opened this issue Jun 18, 2012 · 18 comments

Comments

@vicuna
Copy link

vicuna commented Jun 18, 2012

Original bug ID: 5652
Reporter: @bobzhang
Status: resolved (set by @damiendoligez on 2017-03-03T11:07:05Z)
Resolution: suspended
Priority: normal
Severity: feature
Category: -for ocamlbuild use https://github.com/ocaml/ocamlbuild/issues
Tags: patch
Monitored by: @avsm

Bug description

This is my ad-hoc solution, ocaml_specific.mli does not export anything, so I
have to copy paste.
The speed improvement is obvious. I gained 2~3 times speed improvement

(stolen from Ocaml_specific.ml)
let camlp4_flags camlp4s =
List.iter begin fun camlp4 ->
flag ["ocaml"; "pp"; camlp4] (A camlp4)
end camlp4s;;
camlp4_flags ["camlp4o.opt"; "camlp4r.opt"; "camlp4of.opt"; "camlp4rf.opt"; "camlp4orf.opt"; "camlp4oof.opt"];;
let camlp4_flags' camlp4s =
List.iter begin fun (camlp4, flags) ->
flag ["ocaml"; "pp"; camlp4] flags
end camlp4s;;
camlp4_flags' ["camlp4orr.opt", S[A"camlp4of.opt"; A"-parser"; A"reloaded"];
"camlp4rrr.opt", S[A"camlp4rf.opt"; A"-parser"; A"reloaded"]];;
let camlp4 ?(default = A "camlp4r.opt") ?(printer=A "r")
tag i o env build = (
let ml = env i and pp_ml = env o in
let tags = (((tags_of_pathname ml) ++ "ocaml" ++ "pp") ) ++ tag in
let _deps = Rule.build_deps_of_tags build (tags ++ "ocamldep") in
let pp = Command.reduce (Flags.of_tags tags) in
let pp = match pp with | N -> default | _ -> pp in
Cmd (S [ pp; P ml; A "-printer";printer; A "-o"; Px pp_ml ])
)

File attachments

@vicuna
Copy link
Author

vicuna commented Jun 18, 2012

Comment author: meyer

Patch looks sensible with a reservation that ocaml.opt needs to be on path, so it should be rather:

flag ["ocaml"; "pp"; "native"; camlp4] (A camlp4)

EDIT:
Now I found that solver handles if it should pick up the ".opt" version of the toolchain, so I suppose the place that should be looked is options.ml

@vicuna
Copy link
Author

vicuna commented Jun 18, 2012

Comment author: @bobzhang

yes, but options has nothing related with camlp4. It would be nice to export it

@vicuna
Copy link
Author

vicuna commented Jul 10, 2012

Comment author: @bobzhang

make a change in options.ml is reasonalble
Options.camlp4

Since users can make their own camlp4 driver and won't bother myocamlbuild.ml

@vicuna
Copy link
Author

vicuna commented Sep 2, 2012

Comment author: meyer

Checked in a fix in r12902.

The user still could still use -use-ocamlfind option and the "syntax(camlp4o.opt)" tag.

@vicuna
Copy link
Author

vicuna commented Sep 2, 2012

Comment author: @bobzhang

will this fix cause such a problem
camlp4o.opt ff.cma
?

@vicuna
Copy link
Author

vicuna commented Sep 2, 2012

Comment author: meyer

Sorry, I don't understand.

The fix works by filling holes with the camlp4 flavors with a preference on .opt version of these. If the .opt version is not available in the system it will fallback to the bytecode version.

There is a workaround that had been working without the fix that I checked in by using ocamlfind -syntax option (the proposed: syntax(camlp4o.opt))

However perhaps we need to wrap syntax in a nicer way, avoiding explicitly specifying .opt version, and let the ocamlbuild to chose the right one.

@vicuna
Copy link
Author

vicuna commented Sep 10, 2012

Comment author: meyer

hongboz could you try the latest trunk and confirm the issue is gone?

Thanks

@vicuna
Copy link
Author

vicuna commented Sep 10, 2012

Comment author: @bobzhang

meyer, I will have a try after ICFP

@vicuna
Copy link
Author

vicuna commented Dec 20, 2012

Comment author: @bobzhang

This commit gives no way to use camlp4o instead of camlp4o.opt when you only have pa_*.cmo instead....

@vicuna
Copy link
Author

vicuna commented Dec 21, 2012

Comment author: meyer

hongboz: I'm more than happy to review if you come up with a patch

@vicuna
Copy link
Author

vicuna commented Dec 22, 2012

Comment author: @bobzhang

I have added a simple patch, please have a look

@vicuna
Copy link
Author

vicuna commented Dec 28, 2012

Comment author: @bobzhang

meyer, will you take a look at the patch, if it's okay ?
thanks!

@vicuna
Copy link
Author

vicuna commented Dec 28, 2012

Comment author: @avsm

The patch currently in trunk breaks our build system, since it tries to run "camlp4o.opt" with bytecode plugins instead of native code versions.

Most of the ocamlfind META files don't provide native code alternatives, and so it won't be safe to implicitly switch between the byte/native code versions of camlp4o without patching them first.

Can the patch in trunk be reverted to the old behaviour for the moment, so that package builds don't break?

@vicuna
Copy link
Author

vicuna commented Dec 28, 2012

Comment author: meyer

Good catch, thanks for reporting. Reverted the fix in commit 13167 on trunk.

avsm, could you please monitor issue, as we currently suffer from not enough tests for ocamlbuild (this is being fixed at the moment;) and report back once the problem is fixed properly? I'm going have a go with it over the weekend.

@vicuna
Copy link
Author

vicuna commented Dec 29, 2012

Comment author: @avsm

Sure; I've added trunk snapshots to our Jenkins tests now, so I can test patches fairly easily.

@vicuna
Copy link
Author

vicuna commented Dec 29, 2012

Comment author: meyer

hongboz: your patch does not flag ocaml.opt being "native", so I'm testing slightly modified version now - and adding a test.

@vicuna
Copy link
Author

vicuna commented Jan 19, 2013

Comment author: meyer

I've included your patch in commit 13260.

BTW: Native plugins are not being built with trunk version of Camlp4, can you look at it?

$ ocamlfind ocamlopt -pp 'camlp4o.opt -parser macro -DTEST' dummy.ml  
Camlp4: Uncaught exception: DynLoader.Error ("Camlp4MacroParser.cmxs", "file not found in path")

@vicuna
Copy link
Author

vicuna commented Mar 3, 2017

Comment author: @damiendoligez

ocamlbuild is now a separate project that lives on GitHub.
PR transferred to ocaml/ocamlbuild#210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant