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

On MSVC, link libfoo.lib or foo.lib for -lfoo #32

Merged
merged 1 commit into from Sep 14, 2017

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jun 4, 2017

This is related to ocaml/ocaml#1189, please see the full description of the motivation there!

@alainfrisch
Copy link
Collaborator

I'm not sure to understand exactly the relation with ocaml/ocaml#1189. Is the PR on ocaml still relevant if the current PR on flexdll is accepted?

@dra27
Copy link
Member Author

dra27 commented Aug 12, 2017

Sorry for the lag. Both these are PRs are relevant - ocaml/ocaml#1189 only applies this heuristic ocamlmklib; this PR allows applies the same heuristic to ocamlc and ocamlopt (which of course just pass -l options on to the linker - in this case, flexlink).

reloc.ml Outdated
let r =
match find_file fn with
match List.fold_left (function None -> find_file | acc -> fun _ -> acc) None fns with
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not very readable. Could you introduce some helper function such as val map_find: ('a -> 'b option) -> 'a list -> 'b option (implemented as a recursive function)?

Otherwise, the PR looks good to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed, that's a bit hideous - sorry! Factored out...

@dra27
Copy link
Member Author

dra27 commented Sep 14, 2017

Sorry for the delay addressing the PR point - would it be possible for this to go in and release 0.36 be cut so that 4.06 can declare it as a minimum requirement? It would be good for the special-case treatment of -l to be in sync. It's also of course worth getting a version out including #40)

@alainfrisch alainfrisch merged commit 22888b5 into ocaml:master Sep 14, 2017
@alainfrisch
Copy link
Collaborator

I've created 0.36. I'm not sure about binary releases, though. Previous versions where built with the mingw (32-bit) port of OCaml, which means (i) the resulting flexlink.exe had no dependency on msvcrXXX.dll (good!), but (ii) it was limited by the size of strings in 32-bit OCaml processes (bad!). I think that these days, one can safely assume that OCaml developers under Windows are using a 64-bit OS. Do you agree? So mingw64?

As for the runtime support objects, the msvc32/64 ones were built with a very old version of VS (VS2008 I think), and they would not work with recent versions. Do you have a suggestion?

@dra27
Copy link
Member Author

dra27 commented Sep 14, 2017

Perhaps best to include two versions of flexlink.exe in the binary - 32 and 64? Perhaps have the main one being 64-bit and a subdirectory x86 containing the 32-bit version? I've been using 64-bit since Windows Vista, but you never know...

Not sure how @damiendoligez would feel about our changing the build process for FlexDLL during the 4.06 cycle (so moving to requiring the user to supply the flexdll.c and flexdll_initer.c and a binary flexlink.exe vs providing the object files). I see three options:

  1. "status quo" - binary release as normal (using VS2008) and assume VS 2015/2017 users will either grumble or know to recompile
  2. Include the .c files and ensure that running make support is well-documented (both in OCaml and in FlexDLL)
  3. Change the process in OCaml so that byterun/Makefile expects to find flexdll.c and flexdll_initer.c with flexdll.h and compiles and installs them itself. The compiler would still expect flexlink.exe to be provided (unless you're bootstrapping flexlink, of course).

I like 3, but I can't implement it in the next 24 hours!

@alainfrisch
Copy link
Collaborator

Perhaps best to include two versions of flexlink.exe in the binary - 32 and 64? Perhaps have the main one being 64-bit and a subdirectory x86 containing the 32-bit version?

Do you see an advantage of that over creating two binary releases/installers (which seems rather common under Windows)?

For 2: do you mean, including both the .c files and the Makefile, so that users (with a Cygwin environment) can run "make support" on their side?

A variant could be to add to flexlink.exe the ability to compile its support files (from the .c files found in the same directory as flexlink.exe) using the compilers currently in the PATH. E.g. "flexlink -toolchain msvc -prepare-runtime -o foo/" would create the support objects under foo/.

@dra27
Copy link
Member Author

dra27 commented Sep 14, 2017

Either way for the distribution works fine - for a graphical installer, yes, it'd be more normal to have two (I've never installed flexlink that way!).

Yes, that is what I meant for 2 - on the assumption that the user has a Cygwin environment because they're just about to compile OCaml. That of course assumes that any Windows binary distribution of OCaml therefore includes FlexDLL (kinda true, but only by coincidence!).

Not sure about having flexlink being able to spit out the runtime objects - would you then expect ocamlc/ocamlopt to run as necessary, or expect the user to have done it beforehand?

@alainfrisch
Copy link
Collaborator

would you then expect ocamlc/ocamlopt to run as necessary, or expect the user to have done it beforehand?

The OCaml's build system (resp. the future ./configure for Windows) would generate the runtime objects, and install them together with OCaml libraries.

@dra27
Copy link
Member Author

dra27 commented Sep 14, 2017

Ah, OK - so it's similar to what I propose in 3, but in a more structured manner - OCaml simply invokes a command which tells flexlink to output its objects, rather than assuming that it can compile the .c files itself. Nice!

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

2 participants