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

buildOcaml: remove camlp4 dependency #65632

Closed
wants to merge 1 commit into from

Conversation

@Zimmi48
Copy link
Member

Zimmi48 commented Jul 31, 2019

Motivation for this change

This removes the dependency on camlp4 for most packages that are built with buildOcaml, making it easier to test these packages on recent versions of OCaml where camlp4 might not yet be available.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Running nix-shell -p nix-review --run "nix-review wip" allowed to test this change against the following list of packages and their dependencies (but some OCaml packages have still probably been missed in the process):

alt-ergo framac fstar google-drive-ocamlfuse jackline libbap ocamlPackages.asn1-combinators ocamlPackages.async_ssl ocamlPackages.bap ocamlPackages.cryptokit ocamlPackages.ctypes ocamlPackages.eliom ocamlPackages.email_message ocamlPackages.erm_xmpp ocamlPackages.gapi_ocaml ocamlPackages.git ocamlPackages.git-http ocamlPackages.git-unix ocamlPackages.js_build_tools ocamlPackages.llvm ocamlPackages.merlin_extend ocamlPackages.nocrypto ocamlPackages.ocsigen-start ocamlPackages.ocsigen-toolkit ocamlPackages.ocsigen_server ocamlPackages.otr ocamlPackages.reason ocamlPackages.tls ocamlPackages.tsdl ocamlPackages.x509 ocamlPackages.zarith opa python27Packages.bap python37Packages.bap trv why3

Notify maintainers

cc @vbgl

This removes the dependency on camlp4 for most packages that are built
with buildOcaml, making it easier to test these packages on recent
versions of OCaml where camlp4 might not yet be available.
@vbgl

This comment has been minimized.

Copy link
Contributor

vbgl commented Jul 31, 2019

IMHO it’s a waste. buildOcaml is legacy that we should not maintain.

Are there really packages that use buildOcaml and are meant to be compatible with recent versions of OCaml? It should be more valuable to just fix them.

@Zimmi48

This comment has been minimized.

Copy link
Member Author

Zimmi48 commented Aug 8, 2019

I was prompted by a user of the #ocaml channel on the ReasonML Discord trying to update OCaml packages to use OCaml 4.08. But yes, for sure we should try to remove most packages using buildOcaml instead: these are often older versions of packages that I suppose are kept because of some reverse dependencies but I don't know how to check this.

@vbgl vbgl mentioned this pull request Aug 8, 2019
3 of 10 tasks complete
@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Aug 8, 2019

I added 4.08 support to campl4 here #66343.
But I think this is still worthwhile to remove campl4, the project mentioned that this will be the last release, so it won't be available >4.08 anyway

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Aug 8, 2019

I will say that having a common helper for building a package for ocaml stil helps in dealing with ocaml-specific boiler-plate/standardization problems such as:

  • consistent naming of packages
  • setting environment variables such as CAML_LD_LIBRARY_PATH
  • setting default needed packages E.g ocaml, ocamlbuild
@Zimmi48 Zimmi48 closed this Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.