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

Provide for linking the native toplevel now exposed as 'ocamltoplevel.cmxa' #10

Merged
merged 2 commits into from
Mar 15, 2021

Conversation

AltGr
Copy link
Member

@AltGr AltGr commented Jan 15, 2021

See ocaml/ocaml#10124

This is clearly not optimal, since it will force linking to the native
backend and dynlink even for bytecode mode ; is there a way to specify
requires(byte) and requires(native) ?

@AltGr
Copy link
Member Author

AltGr commented Jan 20, 2021

An alternative which may be better is to provide it as an alternate package:

modified   site-lib-src/compiler-libs/META.in
@@ -30,10 +30,16 @@
 `)'
 
 `package "toplevel" ('
-`  requires = "compiler-libs.bytecomp compiler-libs.optcomp dynlink"'
+`  requires = "compiler-libs.bytecomp"'
 `  version = "[distributed with Ocaml]"'
 `  description = "Toplevel interactions"'
 `  archive(byte) = "ocamltoplevel.cma"'
+`)'
+
+`package "native-toplevel" ('
+`  requires = "compiler-libs.bytecomp compiler-libs.optcomp dynlink"'
+`  version = "[distributed with Ocaml]"'
+`  description = "Toplevel interactions"'
 `  archive(native) = "ocamltoplevel.cmxa"'
 `)'

So consumers of the current toplevel won't see a change of behaviour, and it's easy to switch to try a native build.

@gerdstolpmann
Copy link
Contributor

@AltGr unfortunately there is no requires(byte) and requires(native) (only one deps tree), so I think the separate package is the better way to go.

@emillon
Copy link

emillon commented Jan 26, 2021

I'm not sure how versioning works in ocamlfind, but does this change in the installed META file happen for all compiler versions?

@gerdstolpmann
Copy link
Contributor

@emillon if the META files have to be different by compiler version, the only way is to check the version in the configure script and generate the META file from that.

@AltGr
Copy link
Member Author

AltGr commented Jan 28, 2021

Updated with the separate version, where I added an exists_if so that it shouldn't need modifying depending on the OCaml version.

AltGr added a commit to AltGr/dune that referenced this pull request Jan 28, 2021
AltGr added a commit to AltGr/dune that referenced this pull request Jan 28, 2021
As per ocaml/ocamlfind#10

Signed-off-by: Louis Gesbert <louis.gesbert@ocamlpro.com>
AltGr added a commit to AltGr/dune that referenced this pull request Jan 28, 2021
As per ocaml/ocamlfind#10

Signed-off-by: Louis Gesbert <louis.gesbert@ocamlpro.com>
AltGr added a commit to AltGr/dune that referenced this pull request Jan 28, 2021
As per ocaml/ocamlfind#10

Signed-off-by: Louis Gesbert <louis.gesbert@ocamlpro.com>
@ghost
Copy link

ghost commented Feb 11, 2021

@gerdstolpmann does the current state of the PR looks good to you? The corresponding changes to OCaml have now been merged.

rgrinberg pushed a commit to AltGr/dune that referenced this pull request Feb 14, 2021
As per ocaml/ocamlfind#10

Signed-off-by: Louis Gesbert <louis.gesbert@ocamlpro.com>
Copy link
Contributor

@gerdstolpmann gerdstolpmann left a comment

Choose a reason for hiding this comment

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

Looks good

rgrinberg pushed a commit to ocaml/dune that referenced this pull request Mar 7, 2021
As per ocaml/ocamlfind#10

Signed-off-by: Louis Gesbert <louis.gesbert@ocamlpro.com>
@gerdstolpmann gerdstolpmann merged commit b492ab0 into ocaml:master Mar 15, 2021
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.

None yet

3 participants