Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005435OCamlOCamlbuild (the tool)public2011-12-18 19:302013-08-31 12:48
Reporterjjb 
Assigned Toprotz 
PrioritynormalSeverityminorReproducibilityalways
StatusclosedResolutionfixed 
PlatformAnyOSWindowsOS VersionAny
Product Version3.12.1 
Target VersionFixed in Version4.01.0+dev 
Summary0005435: ocamlbuild does not find .opt executables on Windows
DescriptionOcamlbuild 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.
TagsNo tags attached.
Attached Files? file icon patch-ocamlbuild-opt [^] (3,852 bytes) 2012-03-29 10:33 [Show Content]
patch file icon 0001-PR-5435-search_in_path-return-the-exact-filepath-fou.patch [^] (4,652 bytes) 2012-04-02 10:26 [Show Content]

- Relationships

-  Notes
(0006390)
protz (manager)
2011-12-19 15:21

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
(0006408)
jjb (reporter)
2011-12-20 12:27

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
(0006555)
doligez (administrator)
2011-12-29 21:06

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
(0007213)
protz (manager)
2012-03-28 15:21

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.
(0007222)
protz (manager)
2012-03-29 10:34

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
(0007232)
protz (manager)
2012-03-29 11:41

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 :).
(0007235)
xclerc (developer)
2012-03-29 13:22

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.
(0007236)
protz (manager)
2012-03-29 14:36

Committed r12293
(0007237)
protz (manager)
2012-03-29 14:37

I'll commit this on branch after a while, barring any major objections.
(0007238)
jjb (reporter)
2012-03-29 15:58

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.
(0007252)
protz (manager)
2012-03-30 17:18

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!
(0007258)
gasche (developer)
2012-04-02 09:41

> 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.
(0007259)
gasche (developer)
2012-04-02 10:28

> 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.
(0007264)
protz (manager)
2012-04-02 17:19

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

- Issue History
Date Modified Username Field Change
2011-12-18 19:30 jjb New Issue
2011-12-19 15:21 protz Note Added: 0006390
2011-12-20 12:27 jjb Note Added: 0006408
2011-12-29 21:06 doligez Note Added: 0006555
2012-02-02 15:17 protz Category OCamlbuild => OCamlbuild (the tool)
2012-03-26 15:26 lefessan Assigned To => protz
2012-03-26 15:26 lefessan Status new => acknowledged
2012-03-28 15:21 protz Note Added: 0007213
2012-03-29 10:33 protz File Added: patch-ocamlbuild-opt
2012-03-29 10:34 protz Note Added: 0007222
2012-03-29 11:41 protz Note Added: 0007232
2012-03-29 13:22 xclerc Note Added: 0007235
2012-03-29 14:36 protz Note Added: 0007236
2012-03-29 14:36 protz Status acknowledged => resolved
2012-03-29 14:36 protz Fixed in Version => 4.01.0+dev
2012-03-29 14:36 protz Resolution open => fixed
2012-03-29 14:37 protz Note Added: 0007237
2012-03-29 15:58 jjb Note Added: 0007238
2012-03-30 17:18 protz Note Added: 0007252
2012-04-02 09:41 gasche Note Added: 0007258
2012-04-02 10:26 gasche File Added: 0001-PR-5435-search_in_path-return-the-exact-filepath-fou.patch
2012-04-02 10:28 gasche Note Added: 0007259
2012-04-02 17:19 protz Note Added: 0007264
2013-08-31 12:48 xleroy Status resolved => closed


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker