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

quick windows fixes for 4.03 #70

Closed
wants to merge 3 commits into from
Closed

quick windows fixes for 4.03 #70

wants to merge 3 commits into from

Conversation

fdopen
Copy link
Contributor

@fdopen fdopen commented Apr 12, 2016

three windows related changes - feel free to cherry-pick:

My_unix.run_and_open will either call the internal My_unix.run_and_open (

let run_and_open s kont =
) or Ocamlbuild_unix_plugin.run_and_open (
let run_and_open s kont =
).

The first function will use bash, the second plain cmd.exe. That's confusing. You can't always know, which function will be called, but you have to quote differently for both cases (Filename.quote vs Shell.quote_filename_if_needed). It can happen e.g. here:

let camlp4dir =
2>/dev/null is obviously wrong for cmd.exe, but bash.exe will accept it.

Proper shell quoting. The code is copy&pasted from the ocaml source tree, because it is not exported. (See http://caml.inria.fr/mantis/view.php?id=6287 )

Another side effect of allowing file names that starts with '_'. Hygiene.check now complains frequently about files under '_build/' (eg. myocamlbuild.cmi). The default build dir is: let build_dir = ref (Filename.concat (Sys.getcwd ()) "_build"). Filename.concat always use backslashes as path separator. My_std.filename_concat (/) however uses slashes, therefore the comparison here (

&& ((* beware: !Options.build_dir is an absolute directory *)
) won't return the desired result. (see ocaml/opam-repository#6224 )

I've just changed Pathname.normalize, that previously didn't work reliable on windows at all, because only slashes were treated as path separators. It now always return a version of the file name with an uppercase drive letter and only forward slashes. (I've first tried to return a string with backslashes, but the solver didn't like it.)

"bash --norc -c " ^ Filename.quote s
else
s
in
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for not calling sys_command directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is implem.run_and_open overwritten in the first place?
I've assumed to avoid a temporary file, if Unix.open_process_in is used instead of Sys.command.

@gasche
Copy link
Member

gasche commented Apr 12, 2016

I don't know anything about windows programming so I won't be able to review this PR. @whitequark or @dra27, any suggestions?

gasche added a commit that referenced this pull request Apr 30, 2016
the definition of Options.build_dir, using `Filename.concat`, and the
test in Slurp that uses `filename_concat`. On a Unix those two
functions are the same, so the bug is only on Windows where they
differ.

The previous code had been written to solve MPR#4502.
@gasche
Copy link
Member

gasche commented Apr 30, 2016

@fdopen, I would like to make a release for the new 4.03-specific flags, and I am not sure which portions of this PR to include in it. In the end, I would like to include everything, but I haven't had any feedback from other people using ocamlbuild on Windows, so I won't include the more invasive changes right now.

I wrote a smaller patch (pushed in master) trying to fix the detection of the build directory in the hygiene check, b29023c . Could you maybe give it a try and say if it suffices to solve the problem (3) on your machine?

@msprotz
Copy link

msprotz commented Jun 1, 2016

  1. and 2) are good but I would like to see 2) fixed properly, a.k.a. export unix_quote in the standard library; just hide it after (** / **) so that it doesn't appear in the doc if it's controversial

Please be aware that there's a MASSIVE caveat with running bash -c (see http://stackoverflow.com/questions/9946586/cygwin-bash-c-yields-different-argc-depending-on-whether-the-command-start-with) and that this has bitten us in the past (that's the reason why I added '' in front of every command launched by ocamlbuild via its bash-on-windows invocation).

avsm added a commit to avsm/opam-repository that referenced this pull request May 28, 2023
This patch has been lingering externally since 2016
(ocaml/ocamlbuild#70), and Windows builds
don't get very far without it any more, and it's blocking development
of other upstream Windows ports (ocaml-multicore/eio#530).

So barring strong objections, I'd like to pull this into opam-repository
mainline. No version bump required since it only patches Windows, which is
broken anyway without this fix.

Source patch from https://github.com/fdopen/opam-repository-mingw/blob/opam2/packages/ocamlbuild/ocamlbuild.0.14.2/opam
avsm added a commit to avsm/opam-repository that referenced this pull request May 29, 2023
A variant of this patch has been lingering externally since 2016
(ocaml/ocamlbuild#70), and Windows builds don't get very far without it
any more, and it's blocking development of other upstream Windows ports
(ocaml-multicore/eio#530).

So barring strong objections, I'd like to pull this into opam-repository
mainline.  This package is only available on Windows.

Source patch from https://github.com/fdopen/opam-repository-mingw/blob/opam2/packages/ocamlbuild/ocamlbuild.0.14.2/opam

Reviewed by @dra27
avsm added a commit to avsm/opam-repository that referenced this pull request May 29, 2023
A variant of this patch has been lingering externally since 2016
(ocaml/ocamlbuild#70), and Windows builds don't get very far without it
any more, and it's blocking development of other upstream Windows ports
(ocaml-multicore/eio#530).

So barring strong objections, I'd like to pull this into opam-repository
mainline.  This package is only available on Windows.

Source patch from https://github.com/fdopen/opam-repository-mingw/blob/opam2/packages/ocamlbuild/ocamlbuild.0.14.2/opam

Reviewed by @dra27 and @patricoferris
@hhugo
Copy link
Collaborator

hhugo commented May 29, 2024

(1) was included in #329
(2) is part of #333
(3) probably no longer needed now that we have b29023c

@hhugo hhugo closed this Jun 20, 2024
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.

4 participants