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
ocamlbuild does not find .opt executables on Windows #5435
Comments
Comment author: @protz Hi, Thanks for the patch. Some comments, from a person who's not an ocamlbuild expert.
Thanks, jonathan |
Comment author: @jberdine Thanks for looking at this. First, I am not an ocamlbuild expert, I am just chasing down the source of differences in behavior I see across platforms.
Since the constructed paths are manipulated by Filename.dirname and Filename.basename (see sys_file_exists), and these Filename functions expect Filename.dir_sep, unconditionally using '/' in filename_concat causes the Filename functions to return "incorrect" results. So I don't think that relying on a shell to parse the slashes is going to work (stronger actually, I observe it not finding ocamlc.opt). On a related note, if I change filename_concat in my_std.ml to: let filename_concat x y = then I can't build ocaml on Windows, although other systems work. Any suggestions would be very welcome. Cheers, Josh |
Comment author: @damiendoligez Just a note about / under windows: it's a little known fact that the Windows kernel handles them fine (they are equivalent to ) The only problem with using / under Windows is when you build a command line for some Windows executable that tries to interpret them as beginning a command-line option, and not as directory separators. So if you use / instead of dir_sep, the system calls (like Sys.file_exists) and the Filename.{basename,dirname} functions should still work correctly, even under Windows. Cheers, Damien |
Comment author: @protz Ok, I'm looking into it right now, but just for the record, the latest installer for windows (http://yquem.inria.fr/~protzenk/caml-installer/ocaml-4.01.0+dev0-i686-mingw64.exe) provides a fully-working ocamlfind, with ocamlopt parameterized as ocamlopt.opt. So simply calling ocamlbuild -use-ocamlfind will solve your issue. |
Comment author: @protz I believe the patch above fixes the issue in a clean way. Xavier, could you please review the patch and make sure I haven't done any suspicious changes? Thanks, jonathan |
Comment author: @protz jjb, let me know if you have strong opinions in favor of your method or mine. Please note that the following line:
will actually not do anything meaningful, because ocamlbuild is unable to properly split the PATH components. On Windows, because a Win32 OCaml directly uses the Windows-provided getenv, the PATH will be ;-separated, not :-separated, so I still think we need to fix that (this is part of my patch). You probably didn't run into this use case because Ocamlbuild_where.bindir also happens to be the directory you installed ocaml in. However, this isn't true anymore if the user uses, say, the OCaml installer for Windows. The remaining question is: should we try to fix the problem in [search_in_path] or [mk_virtual_solvers]. I'm unsure :). |
Comment author: @xclerc The patch attached to this issue seems very reasonable to me. As pointed out by gasche, [search_in_path] is exported (through the plugin |
Comment author: @protz Committed r12293 |
Comment author: @protz I'll commit this on branch after a while, barring any major objections. |
Comment author: @jberdine Perhaps I have missed something, but don't the calls to [sys_file_exists] in [mk_virtual_solvers] also need to look for ".exe", using the same [exists] function you have in [search_in_path]? I am thinking of the case where [Ocamlbuild_where.bindir] exists and there is a different version of OCaml installed in the PATH. If the branch on [os_type] in [env_path] was moved into [parse_environment_path], would the other clients of [parse_environment_path] get fixed on Windows with this same change? Perhaps not important, but the change to filename_concat makes it almost an inlined version of Filename.concat. Have you understood why it is different? I isn't clear to me, and feels like a lurking bug waiting to bite... Regarding the comment about not adding the ".exe" extension to the executable file name, the Windows shell will get confused if there are executable files named e.g. "foo" and "foo.exe". This may not arise in this context, but it may be clearer to just add the extension and avoid depending on the shell's behavior. |
Comment author: @protz You are right about also checking for .exe versions in [mk_virtual_solvers]. However, I'm getting strange errors when trying to build a package with odb after fixing this, and I think fixing this bug may have exposed another one. I need to further investigate! |
Comment author: @gasche
For the record, I have sent an e-mail to the original authors of the code (Berke and Nicolas) to get more information on this (indeed) fishy point. I hope to clarify this and avoid, or at least document carefully, this duplication of concern. We'll see how it goes. |
Comment author: @gasche
Just in case this is considered important, I uploaded a small patch on top of protz's patch submission, to return the exact filepath with the .exe extension. This was not done before because it is not convenient with stdlib's List.find function. Not sure if it brings much, though. |
Comment author: @protz I've also committed r12305 after spending the afternoon trying to fix this with Damien. Committed on the 4.00 branch as r12307 and r12308. After discussing the matter with Damien, we feel like it's not really important trying to make a distinction between having the .exe suffix or not. Cheers, jonathan |
git-svn-id: http://caml.inria.fr/svn/ocaml/version/4.00@12307 f963ae5c-01c2-4b8c-9fe0-0dff7051ff02
Original bug ID: 5435
Reporter: @jberdine
Assigned to: @protz
Status: closed (set by @xavierleroy on 2013-08-31T10:48:56Z)
Resolution: fixed
Priority: normal
Severity: minor
Platform: Any
OS: Windows
OS Version: Any
Version: 3.12.1
Fixed in version: 4.01.0+dev
Category: -for ocamlbuild use https://github.com/ocaml/ocamlbuild/issues
Monitored by: @gasche @protz @ygrek @xclerc
Bug description
Ocamlbuild searches for e.g. ocamlc.opt, while on Windows it should instead look for ocamlc.opt.exe. Here is a modified version of mk_virtual_solvers from ocamlbuild/options.ml:
let mk_virtual_solvers =
let dir = Ocamlbuild_where.bindir in
List.iter begin fun cmd ->
let opt = cmd ^ ".opt" in
let cmd_exe = cmd ^ !exe in
let opt_exe = opt ^ !exe in
let a_opt = A opt_exe in
let a_cmd = A cmd_exe in
let search_in_path = memo Command.search_in_path in
let solver () =
if sys_file_exists !dir then
let long = Filename.concat !dir cmd in
let long_exe = long ^ !exe in
let long_opt = long ^ ".opt" in
let long_opt_exe = long_opt ^ !exe in
if sys_file_exists long_opt_exe then A long_opt_exe
else if sys_file_exists long_exe then A long_exe
else try let _ = search_in_path opt_exe in a_opt
with Not_found -> a_cmd
else
try let _ = search_in_path opt in a_opt
with Not_found -> a_cmd
in Command.setup_virtual_command_solver (String.uppercase cmd) solver
end
This requires the 'exe' value from further down the file to be moved up. Note that since filename_concat in ocamlbuild/my_std.ml is broken on Windows, Filename.concat is used here. Also, filename_concat also breaks search_in_path, so if the executables are in a directory other than the compiled-in default, they will not be found.
File attachments
The text was updated successfully, but these errors were encountered: