Skip to content

Conversation

@hhugo
Copy link
Member

@hhugo hhugo commented Oct 22, 2013

review are welcome

This was referenced Oct 22, 2013
@gasche
Copy link

gasche commented Oct 25, 2013

(I'm not a js_of_ocaml contributor or anything, so I'm only giving a purely personal opinion here.)

I'm impressed by the level of ocamlbuild expertise displaying in this myocamlbuild.ml.

However, there is one small part where I think is going too far: the install/uninstall rule. Encoding them inside ocamlbuild is a clever trick, but I'm not sure it's actually a good idea on the long term. Indeed, while ocamlbuild systems can gain in concision and expressivity when compared to Makefile, they also require a more specialized skillset to maintain and modify, and this can be a barrier to user. I think it's important that the install/uninstall rules remain easy to tweak by non-project-expert, typically packagers for exotic systems you don't know about. So I would suggest that you keep using simple shell scripting in the Makefile for deploying the project on the system -- possibly at the cost of slightly more information duplication, I'm not sure I understand fully the move from in-makefile list of files to those .install files.. Ocamlbuild really shines on the building part, anyway.

That said, there have been some requests from users to have ocamlbuild help a bit with the "install" part of the project (not to do it itself, but help install tools do their work). See for example PR#6067 by Daniel Bünzli. I would be happy to have any feedback (or code/patch contribution) on these issues.

@vouillon
Copy link
Member

Ocamlbuild seems kind of slow, though.
make -j examples takes 7s with the previous Makefile and 37s with ocamlbuild on my machine.

@kit-ty-kate
Copy link
Contributor

The problem is: ocamlbuild is not able to parallelize :/ It's one of the defuncts of ocamlbuild but some people are working on it.

@hhugo
Copy link
Member Author

hhugo commented Oct 25, 2013

  • I agree that the install logic should not go in. (this was more an experiment). I guess it would be easy to move to oasis from here.
  • regarding the speed:
    • examples compilation times could be improved using native camlp4 extension (in both case).
    • Is it an issue anyway ? It is no for me if I gain enough of simplicity

@kit-ty-kate
Copy link
Contributor

examples compilation times could be improved using native camlp4 extension (in both case).

Is this really possible ? Why does OASIS generates only byte files for syntax libraries. @gildor478 ?

@gildor478
Copy link

syntax extension is compiled like library, there is no reason it doesn't create .cmxs just as for library (i.e. native files for syntax libraries).

But maybe the problem may be that camlp4o is will not use .cmxs ? (Never tried).

@kit-ty-kate
Copy link
Contributor

Ok, it's compiled and installed, but not referenced in the META

@gildor478
Copy link

=> this is a bug (probably 1 line in the META plugin if you want to have a look, or open a bug)

@kit-ty-kate
Copy link
Contributor

Ok, it's documented here, but it seems that plugin predicate isn't documented (in the ocamlfind's TODO). Should I handle it ? I'll finish the patch tomorrow.

@gildor478
Copy link

What is the plugin predicate ?

@kit-ty-kate
Copy link
Contributor

The plugin in, for instance: archive(native, plugin) = "oasis.cmxs"

@gildor478
Copy link

OK, I was not sure about what you mean, concerning the plugin predicate. This is not documented in findlib, because it is not a standard predicate. Look at oasis/src/ext/plugin-loader/src/PluginLoad.ml line 148.

The idea was to have something like toploop but for plugin and I use the generic framework offered by findlib to use it. Look at the initial mailing list message:
http://caml.inria.fr/pub/ml-archives/caml-list/2008/11/a671e1378f470e75c5556591973612e0.en.html

The followup on this work is to ping Gerd to include this kind of plugin loading with the camlp4o syntax flag for findlib. (this will give you access to native plugin + native version of camlp4).

@hhugo
Copy link
Member Author

hhugo commented Oct 29, 2013

it seams that ocamlbuild doesn't handle native camlp4 syntax extensions correctly.

I use to wrap camlp4 calls with a script that either call camlp4/camlp4o/camlp4o.opt/camlp4r/camlp4r.opt/.. depending on whether native versions of libs are available. (hiding camlp4 binary using PATH env variable)

I don't know how hard it would be to fix ocamlbuild. (one try here ocaml/ocaml@655cf17)

@hhugo
Copy link
Member Author

hhugo commented Nov 1, 2013

@vouillon, would you consider merging this after the INSTALL is fixed/cleaned ?

@hhugo
Copy link
Member Author

hhugo commented Nov 2, 2013

@gasche, PR#6067 would actually be very helpful.

@vouillon
Copy link
Member

vouillon commented Nov 2, 2013

I don't really care. So, if you think this is an improvement, just go ahead and merge this.

@Drup
Copy link
Member

Drup commented Nov 2, 2013

@hhugo do you plan to make the switch to oasis after merging ?

@hhugo
Copy link
Member Author

hhugo commented Nov 2, 2013

I don't know. I could switch to oasis BEFORE the merge, so I don't have to rewrite the installation part.
It depends on what people agree on. (#25)

@hhugo
Copy link
Member Author

hhugo commented Nov 5, 2013

I've a commit to switch to to oasis hhugo@2791cdb (need more test)

Choose a reason for hiding this comment

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

Beware that this function is not tail-recursive and will fail on big files. I would argue that the imperative style alternative is better:

let lines = ref [] in try
while true do lines := input_line ic :: !lines; done; assert false
with End_of_file -> List.rev !lines

@hhugo
Copy link
Member Author

hhugo commented May 6, 2014

updated version available here
#156

@hhugo hhugo closed this May 6, 2014
@hhugo hhugo deleted the ocamlbuild branch May 6, 2014 17:53
vouillon added a commit that referenced this pull request Oct 29, 2024
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.

7 participants