search

ocaml / flexdll

a dlopen-like API for Windows
Other
97 stars 30 forks source link

Implement issue #72 "Support gcc "-Wl" linker pass through option" #73

Closed MSoegtropIMC closed 4 years ago

MSoegtropIMC commented 5 years ago

This PR implements issue #72.

yakobowski commented 5 years ago

This seems very desirable indeed.

MSoegtropIMC commented 5 years ago

I somehow lost track of this PR - need to check why PR fails here. We are using this patch in Coq since quite a while and it works fine there.

MSoegtropIMC commented 5 years ago

The CI failure is due to incompatibility with OCaml master. It is the same failure as all other recent tests have. See e.g. https://ci.appveyor.com/project/alainfrisch/flexdll/build/1.0.90 So from my point of view this PR is good for integration.

dra27 commented 4 years ago

This is a good idea - would you have a chance to rebase it (CI should then pass - the failure was indeed unrelated), but could you also a note about -Wl, options being passed to the linker in Cmdliner.footer.

I think it's acceptable for these to be passed on verbatim to the MSVC linker as well - in cases where it's a common option it'll work, and we can deal with actually translating options as and when the need arises.

MSoegtropIMC commented 4 years ago

Sure I will rebase and look and look into -Wl

dra27 commented 4 years ago

I took the liberty of rebasing this - @alainfrisch, does it look OK to you?

MSoegtropIMC commented 4 years ago

I took the liberty of rebasing this - @alainfrisch, does it look OK to you?

Thanks - I appreciate it!

Your comment means that I should update the documentation?

Sorry for the delays - I am quite busy these days.

dra27 commented 4 years ago

@MSoegtropIMC - that's no problem at all! The comment I'd left was to mention that -Wl, is supported when you invoke flexlink -help which I've done in https://github.com/alainfrisch/flexdll/pull/73/commits/20f0d493d74ce6c78172fe4bf9d305a081426272

MSoegtropIMC commented 4 years ago

which I've done in 20f0d49

Thanks! One might want to mention that one can pass only one argument with -Wl at a time - several comma separated arguments as the syntax might suggest and gcc supports it - are not supported. Or I (or someone) could fix this ...

MSoegtropIMC commented 4 years ago

P.S.: One can give of cause several -Wl arguments.

dra27 commented 4 years ago

Ah, that's a good point - is that simply a matter of splitting on , characters. or does one have to be more intelligent than that w.r.t. any embedded strings (e.g. is -Wl,-foo="a,b",-bar also legal?)

dra27 commented 4 years ago

(my impression from https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html#Link-Options is that the splitting is simply done on commas)

MSoegtropIMC commented 4 years ago

Ah, that's a good point - is that simply a matter of splitting on , characters. or does one have to be more intelligent than that w.r.t. any embedded strings (e.g. is -Wl,-foo="a,b",-bar also legal?)

I would think that the quotes are handled by the shell anyway, so that flexlink doesn't see them. So my assumption is that a simple split on , is sufficient. The only question is what happens with double commas or commas at the end, but I think once can do this garbage in - garbage out. The point of this is compatibility with make files targeting gcc and it is sufficient to support what is out there in the wild.

alainfrisch commented 4 years ago

I took the liberty of rebasing this - @alainfrisch, does it look OK to you?

Yes, this LGTM! (Perhaps just update the CHANGES file?)

dra27 commented 4 years ago

If you're happy to push a commit with the String.split, I'll update the compatibility layer so that 4.03 and earlier don't complain!

(noted re changes, @alainfrisch - although I was going to go through the branch history and update it shortly too - I think we're nearly at a collected enough point for 0.38!)

dra27 commented 4 years ago

Actually, the compatibility layer change isn't entirely trivial, but I think that the patch is useful to you even when it can online handle a single argument, right? So I've merged this and put my WIP for the splitting into #85.