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

ocamlbuild does not find .opt executables on Windows #5435

Closed
vicuna opened this issue Dec 18, 2011 · 14 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Dec 18, 2011

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 19, 2011

Comment author: @protz

Hi,

Thanks for the patch. Some comments, from a person who's not an ocamlbuild expert.

  1. I feel like the right place to fix this seems to be the search_in_path function, from command.ml. It even has a comment (* FIXME WINDOWS *).

  2. I'm unsure about your changing to Filename.concat, can you explain a little bit more why this change is correct? In particular, if the command goes through the shell, on cygwin or msys, the forward-slashes may end up being properly parsed (e.g. /c/foo/bar on cygwin)...

Thanks,

jonathan

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 20, 2011

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.

  1. I agree that search_in_path should also be fixed, but it looks to me like it's problem is using filename_concat. Note that the conditional in the body of List.find is redundant with the test done in filename_concat. But independently, mk_virtual_solvers calls sys_file_exists directly looking for files without the !exe extension.

  2. I certainly do not fully understand ocamlbuild's internal handling of pathnames. For instance, why does filename_concat in my_std.ml even exist, instead of using Filename.concat (there is a comment: (* FIXME warning fix and use Filename.concat *))? As far as I can tell filename_concat returns relative paths for those within the current working directory, and hardcodes the use of '/' instead of Filename.dir_sep. I do not know if the rest of ocamlbuild depends on either of these two changes.

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 =
if x = Filename.current_dir_name then y else
Filename.concat x y

then I can't build ocaml on Windows, although other systems work. Any suggestions would be very welcome.

Cheers, Josh

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 29, 2011

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 28, 2012

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 29, 2012

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 29, 2012

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:

    else try let _ = search_in_path opt_exe in a_opt

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 :).

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 29, 2012

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
interface) to users. Thus is seems better to have this fix in this very function,
making its enhancement available to plugin writers.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 29, 2012

Comment author: @protz

Committed r12293

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 29, 2012

Comment author: @protz

I'll commit this on branch after a while, barring any major objections.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 29, 2012

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 30, 2012

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!

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 2, 2012

Comment author: @gasche

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...

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 2, 2012

Comment author: @gasche

Regarding the comment about not adding the ".exe" extension to the executablefile 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.

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 2, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.