Use forward slashes and quoted OCaml strings #323

jonahbeckford · 2023-09-19T23:12:33Z

This is the upstream of https://github.com/diskuv/diskuv-opam-repository/blob/main/packages/ocamlbuild/ocamlbuild.0.14.0/files/ocamlbuild-0.14.0.diskuvocaml.patch

Before

Makefile.config:

# This file was generated from configure.make

OCAML_PREFIX=
OCAML_BINDIR=C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/bin
OCAML_LIBDIR=C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/lib/ocaml
OCAML_MANDIR=C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/man

EXT_OBJ=.obj
EXT_ASM=.asm
EXT_LIB=.lib
EXT_DLL=.dll
EXE=.exe

OCAML_NATIVE=true
OCAML_NATIVE_TOOLS=true
NATDYNLINK=true
SUPPORT_SHARED_LIBRARIES=true

PREFIX=Z:\source\ocamlbuild\_opam
BINDIR=Z:\source\ocamlbuild\_opam\bin
LIBDIR=Z:\source\ocamlbuild\_opam\lib
MANDIR=Z:\source\ocamlbuild\_opam\man

src/ocamlbuild_config.ml:

(* This file was generated from ../configure.make *)

let bindir = "Z:\source\ocamlbuild\_opam\bin"
let libdir = "Z:\source\ocamlbuild\_opam\lib"
let ocaml_libdir = "C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/lib/ocaml"
let libdir_abs = "Z:/source/ocamlbuild/_opam/lib"
let ocaml_native = true
let ocaml_native_tools = true
let supports_shared_libraries = true
let a = "lib"
let o = "obj"
let so = "dll"
let ext_dll = ".dll"
let exe = ".exe"
let version = "NEXT_RELEASE"

After

Makefile.config:

# This file was generated from configure.make

OCAML_PREFIX=
OCAML_BINDIR=C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/bin
OCAML_LIBDIR=C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/lib/ocaml
OCAML_MANDIR=C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/man

EXT_OBJ=.obj
EXT_ASM=.asm
EXT_LIB=.lib
EXT_DLL=.dll
EXE=.exe

OCAML_NATIVE=true
OCAML_NATIVE_TOOLS=true
NATDYNLINK=true
SUPPORT_SHARED_LIBRARIES=true

PREFIX=Z:/source/ocamlbuild/_opam
BINDIR=Z:/source/ocamlbuild/_opam/bin
LIBDIR=Z:/source/ocamlbuild/_opam/lib
MANDIR=Z:/source/ocamlbuild/_opam/man

src/ocamlbuild_config.ml:

(* This file was generated from ../configure.make *)

let bindir = {|Z:\source\ocamlbuild\_opam\bin|}
let libdir = {|Z:\source\ocamlbuild\_opam\lib|}
let ocaml_libdir = {|C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/lib/ocaml|}
let libdir_abs = {|Z:/source/ocamlbuild/_opam/lib|}
let ocaml_native = true
let ocaml_native_tools = true
let supports_shared_libraries = true
let a = "lib"
let o = "obj"
let so = "dll"
let ext_dll = ".dll"
let exe = ".exe"
let version = "NEXT_RELEASE"

gasche

This change looks okay to me; I don't know about whether it preserves Windows compatibility, but I certainly assume @jonahbeckford does, and keeping forward slashes for all systems probably makes things simpler.

(On the other hand I don't understand the relation with #321; I will ask there.)

gasche · 2023-09-20T11:54:38Z

In fact, thinking about it twice, I am not completely sure that this is safe for Unix users:

Someone could have a \ in their path, and then the rewrite would silently break things. But this sounds rare, asking for even more trouble than having spaces in the path, so maybe we don't care?
I wonder happens in situations where backslashes have been used to protect about spaces in path. If I am not mistaken, the way the previous code was written would make make -f configure.make OCAML_PREFIX='/tmp/my\ ocaml\ prefix' work correctly (it would generate a script with the content OCAML_PREFIX=/tmp/my\ ocaml\ prefix, which I think is correct), while I would guess that the result with the present PR would be broken.

If (2) is indeed a real issue, we are fixing the behavior of Windows users with backslashes in their paths, but we are breaking the behavior of some Unix users with spaces or other escapable characters in their path, depending on how they are doing their escaping or quoting.

What problems would we have if we only kept the part of this PR that uses quoted string literals, and removed the rotation of slashes?

jonahbeckford · 2023-09-20T13:15:19Z

Test 1

If I am not mistaken, the way the previous code was written would make make -f configure.make OCAML_PREFIX='/tmp/my\ ocaml\ prefix' work correctly (it would generate a script with the content OCAML_PREFIX=/tmp/my\ ocaml\ prefix, which I think is correct), while I would guess that the result with the present PR would be broken.

As best as I can tell after doing a search of the code, the following generated variables in Makefile.config are never used:

OCAML_PREFIX=
OCAML_BINDIR=C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/bin
OCAML_LIBDIR=C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/lib/ocaml
OCAML_MANDIR=C:/Users/beckf/AppData/Local/Programs/DKMLNA~1/man

so the backslash in OCAML_PREFIX is never read. Perhaps we could expand this PR to remove that section from Makefile.config and its generator configure.make?

jonahbeckford · 2023-09-20T13:16:00Z

Test 2 including OCaml quoted string literals

Let's change the example to:

# For repeatability ...
#   We need a workaround since configure.make is highly dynamic:
#     $(shell ocamlc -where)
#     $(shell opam config var ...)
#     $(shell ocamlfind printconf destdir)
#     $(shell ocaml scripts/cat.ml VERSION)
#   1. Ensure [opam] already in PATH
PATH=...:"$PATH"
#   2. Also place [ocamlc], [ocaml] and maybe [ocamlfind] in PATH
eval $(opam env)
#   3. Make shims so that [opam] and [ocamlfind] always fail
WORKDIR=$(mktemp -d)
printf '#!/bin/sh\n exit 1' | tee "$WORKDIR/opam" > "$WORKDIR/ocamlfind"
chmod +x "$WORKDIR/opam" "$WORKDIR/ocamlfind"
PATH="$WORKDIR:$PATH"

# Now generate files
make -f configure.make distclean
make -f configure.make all \
    OCAMLBUILD_PREFIX='/tmp/my\ ocaml\ prefix' \
    OCAMLBUILD_BINDIR='/tmp/my\ ocaml\ prefix/bin' \
    OCAMLBUILD_LIBDIR='/tmp/my\ ocaml\ prefix/lib' \
    OCAMLBUILD_MANDIR='/tmp/my\ ocaml\ prefix/man'
make check-if-preinstalled all

That gives:

# file: Makefile.config
#...

PREFIX=/tmp/my\ ocaml\ prefix
BINDIR=/tmp/my\ ocaml\ prefix/bin
LIBDIR=/tmp/my\ ocaml\ prefix/lib
MANDIR=/tmp/my\ ocaml\ prefix/man

and

(* file:  src/ocamlbuild_config.ml *)

let bindir = {|/tmp/my\ ocaml\ prefix/bin|}
let libdir = {|/tmp/my\ ocaml\ prefix/lib|}
let ocaml_libdir = {|/home/jonahbeckford/.opam/opam-publish/lib/ocaml|}
let libdir_abs = {|/tmp/my\ /home/jonahbeckford/source/ocamlbuild/ocaml\ /home/jonahbeckford/source/ocamlbuild/prefix/lib|}
let ocaml_native = true
(* ... *)

The src/ocamlbuild_config.ml is wrong for two reasons:

The let libdir = {|...|} causes the backslash to appear in ocamlbuild -where which will mess up some users of ocamlbuild:
```
$ ./ocamlbuild.native -where
/tmp/my\ ocaml\ prefix/lib/ocamlbuild
```
That means, with quoted string literals, any backslash will break users
Because Makefiles interpret a space as a list separator, and there is a Makefile expansion abspath in echo 'let libdir_abs = {|$(abspath $(OCAMLBUILD_LIBDIR))|}';, we get a nonsensical three-term let libdir_abs = ....

That means that if you have spaces, some invocations of ocamlbuild will break.

jonahbeckford · 2023-09-20T13:36:34Z

Test 3 without quoted string literals

PATH=...:"$PATH" # Add opam
eval $(opam env)
WORKDIR=$(mktemp -d)
printf '#!/bin/sh\n exit 1' | tee "$WORKDIR/opam" > "$WORKDIR/ocamlfind"
chmod +x "$WORKDIR/opam" "$WORKDIR/ocamlfind"
PATH="$WORKDIR:$PATH"

make -f configure.make distclean
make -f configure.make all \
    OCAMLBUILD_PREFIX='/tmp/some\backspace\dir' \
    OCAMLBUILD_BINDIR='/tmp/some\backspace\dir/bin' \
    OCAMLBUILD_LIBDIR='/tmp/some\backspace\dir/lib' \
    OCAMLBUILD_MANDIR='/tmp/some\backspace\dir/man'
make check-if-preinstalled all

During the compile we get:

File "src/ocamlbuild_config.ml", line 3, characters 32-34:
3 | let bindir = "/tmp/somackspace\dir/bin"
                                    ^^
Warning 14 [illegal-backslash]: illegal backslash escape in string.
File "src/ocamlbuild_config.ml", line 4, characters 32-34:
4 | let libdir = "/tmp/somackspace\dir/lib"
                                    ^^
Warning 14 [illegal-backslash]: illegal backslash escape in string.
File "src/ocamlbuild_config.ml", line 6, characters 36-38:
6 | let libdir_abs = "/tmp/somackspace\dir/lib"
                                        ^^

What is worse is that notice the \b has been encoded as an ASCII backspace character, and now bindir, libdir and libdir_abs are incorrect.

So we generate ...

(* file:  src/ocamlbuild_config.ml *)

let bindir = "/tmp/some�ackspace\dir/bin"
let libdir = "/tmp/some�ackspace\dir/lib"
let ocaml_libdir = "/home/jonahbeckford/.opam/opam-publish/lib/ocaml"
let libdir_abs = "/tmp/some�ackspace\dir/lib"
let ocaml_native = true

and ocamlbuild -where will perform the backspace operation:

$ ./ocamlbuild.native -where
/tmp/somackspace\dir/lib/ocamlbuild

That is the original bug I found in DkML 2.0.3: diskuv/dkml-installer-ocaml#70.

That means backslash characters were already unsafe. Perhaps backslashed spaces work, but if it did that was an accident.

jonahbeckford · 2023-09-20T13:38:32Z

Anyway, I think this needs to be communicated as a major breaking change. ocamlbuild.1.0.0?

gasche · 2023-09-20T13:41:47Z

What I understand from your experiments is that my worries are moot because any use of backslash (be it part of the path, or an attempt to escape spaces) is likely to be deeply broken today.

@kit-ty-kate, I wonder if you have an opinion; do you also now consider that this PR is safe?

kit-ty-kate · 2023-09-21T14:25:57Z

My opinion is that the part changing "..." to {|...|} should always be correct (maybe we could use {path|...|path} to reduce the potential conflicts even more). We only need to bump the version to 0.15.0 and put the change in bold in the changelog just in case someone was using it.

However I’m rather on the fence about the printf '%s' '...' | tr '\' / part. We would have a new problem with files that contain ' (single-quotes) and backslashes on non-windows machines

The let libdir = {|...|} causes the backslash to appear in ocamlbuild -where which will mess up some users of ocamlbuild:

do you have an example of such users?

2. Because Makefiles interpret a space as a list separator, and there is a Makefile expansion abspath in echo 'let libdir_abs = {|$(abspath $(OCAMLBUILD_LIBDIR))|}';, we get a nonsensical three-term let libdir_abs = ....
That means that if you have spaces, some invocations of ocamlbuild will break.

I just read the documentation for the $(abspath ...) builtin function and I’m seriously appalled that it seems to be the only "portable" Makefile builtin provided that does that. I couldn’t find a better non-broken way to do it sadly. Maybe you know of a method that’s actually portable and handles all the possible combinations of characters in files on the different platforms?

gasche · 2023-09-21T15:00:04Z

I agree that quoted strings are fine.

@jonahbeckford I think that I got us in the wrong direction with my previous question on non-Windows peoples using spaces or backslashes in their path. If we ignore those, what are the issues with just using quoted strings and not performing the translation of backslash into slash in this PR?

Note: I also wondered if:

We are trying to support users with a space in their path, which is common under Windows. My understanding is that it would be broken for them (for example due to the abspath issue), but that OCaml is presumably also broken so maybe this is fine. In any case this is outside the scope of the present PR.
We should consider rewriting config.make into an ocaml script. This would make it easier to know what the script is doing in corner cases, but ocamlbuild.opam calls make -f configure.make, and I don't think it is allowed to assume that ocaml exists at this point (it could be a dependency about to be installed).

kit-ty-kate · 2023-09-21T19:00:38Z

We should consider rewriting config.make into an ocaml script. This would make it easier to know what the script is doing in corner cases

Oh i like that very much. Looking at what that file is doing it seems like it would be possible to rewrite it in caml in a more concise way. The only annoying bit seems to be the include $(shell ocamlc -where)/Makefile.config but we could simply replace configure.make by the following Makefile that would print the required values from the included Makefile:

include $(shell ocamlc -where)/Makefile.config

.PHONY: print-BINDIR print-LIBDIR print-MANDIR
print-BINDIR: ; $(info $(BINDIR))
print-LIBDIR: ; $(info $(LIBDIR))
print-MANDIR: ; $(info $(MANDIR))

and we would be able to get the value from OCaml using: make -s -f configure.make print-BINDIR

but ocamlbuild.opam calls make -f configure.make, and I don't think it is allowed to assume that ocaml exists at this point (it could be a dependency about to be installed).

I’m not sure to understand what you mean. We can always assume OCaml is available as every dependencies are required to be installed before the package that depends on them. Plus if that wasn’t the case the Makefile would simply fail (if only for the ocamlc -where)

gasche · 2023-09-21T20:34:05Z

Ah, good point about dependencies being available at package-configure-time already.

The only annoying bit seems to be the include $(shell ocamlc -where)/Makefile.config

We can call ocamlc -config from within an OCaml script to get the same values as found in $(ocamlc -where)/Makefile.config. We also introduced ocamlc -config-var foo in 4.08 that makes it even nicer, if we accept the bump the minimum required version to 4.08. (Currently it is 4.03?!)

jonahbeckford · 2023-09-22T17:31:30Z

However I’m rather on the fence about the printf '%s' '...' | tr '\' / part. We would have a new problem with files that contain ' (single-quotes) and backslashes on non-windows machines

The let libdir = {|...|} causes the backslash to appear in ocamlbuild -where which will mess up some users of ocamlbuild:

do you have an example of such users?

diskuv/dkml-installer-ocaml#70 : ptime. That was my initial blocker, but now that is unblocked. My memory also says I've seen this same error in a few more packages.

I have no bandwidth to do refactoring of ocamlbuild. And there are several more commits from fdopen before ocamlbuild is in a healthy state. If worse comes to worse, let's just keep this issue open with a concrete list of recommendations for an interested party to tackle.

hhugo · 2024-06-20T16:02:16Z

@jonahbeckford would you be able to check if #334 works for your setup ?

jonahbeckford · 2024-06-20T23:44:43Z

@jonahbeckford would you be able to check if #334 works for your setup ?

TLDR: If ocamlbuild compiles ptime.1.1.0 and any other random ocamlbuild package without modification, then it is good.

I think "your setup" meant MSVC on Windows. Has it been tested with opam 2.2 beta3 + MSVC? That is the real target. If that works, this issue can be closed!

hhugo · 2024-06-21T00:04:01Z

#334 is tested with opam 2.2 beta3 + mingw

jonahbeckford · 2024-06-21T00:20:46Z

Oh that is great!

I could have sworn that opam 2.2 has a msvc mode. You may have to ping the opam 2.2 folks how to do that in whatever testing pipeline they gave you. It is possible testing of msvc is punted ... which means this issue should stay open (MSVC is way less tolerant of incorrect slashes and incorrect quotes than GCC/MinGW).

Please don't worry about DkML ... it should work if opam 2.2 + MSVC works (not the other way around). And it has a working fork of ocamlbuild that won't be merged with your changes until absolutely necessary ... which is hopefully never b/c testing an infrastructure package like ocamlbuild is laborious in DkML (well, at least until opam 2.x has parity with DkML ... then DkML won't be needed!).

dra27 · 2024-06-26T07:57:58Z

#347 brings CI for MSVC - I think that although Makefile.config still contains backslashes, the quoted-strings part from this PR extracted in #329 means everything's fixed?

hhugo · 2024-06-26T11:06:26Z

I believe that everything here is fixed

gasche · 2024-06-26T11:37:47Z

I'd like to thank everyone involved here for the end result which is a net improvement to the upstream ocamlbuild windows story (stories, as several distinct configurations are in use). I was of course unhappy that we couldn't help @jonahbeckford get his work upstream, but that required more effort than Jonah and @kit-ty-kate and myself were able to pour into the topic. We are fortunate that @hhugo and @dra27 were willing to revive it, with new Windows CI goodness that made working on this surprisingly efficient. Impressive work, thanks!

gasche approved these changes Sep 20, 2023

View reviewed changes

Use forward slashes and quoted OCaml strings

35c65d9

jonahbeckford force-pushed the win32-use-fwd-slashes branch from 369b374 to 35c65d9 Compare September 20, 2023 13:40

hhugo mentioned this pull request May 16, 2024

Setup github actions for ocamlbuild #328

Merged

hhugo mentioned this pull request May 27, 2024

Enable CI on windows and fix it. #329

Merged

hhugo mentioned this pull request Jun 25, 2024

Add MSVC testing #347

Merged

hhugo closed this in #347 Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use forward slashes and quoted OCaml strings #323

Use forward slashes and quoted OCaml strings #323

jonahbeckford commented Sep 19, 2023

gasche left a comment

gasche commented Sep 20, 2023

jonahbeckford commented Sep 20, 2023

jonahbeckford commented Sep 20, 2023 •

edited

Loading

jonahbeckford commented Sep 20, 2023

jonahbeckford commented Sep 20, 2023

gasche commented Sep 20, 2023

kit-ty-kate commented Sep 21, 2023

gasche commented Sep 21, 2023

kit-ty-kate commented Sep 21, 2023

gasche commented Sep 21, 2023

jonahbeckford commented Sep 22, 2023

hhugo commented Jun 20, 2024

jonahbeckford commented Jun 20, 2024

hhugo commented Jun 21, 2024

jonahbeckford commented Jun 21, 2024 •

edited

Loading

dra27 commented Jun 26, 2024

hhugo commented Jun 26, 2024

gasche commented Jun 26, 2024

Use forward slashes and quoted OCaml strings #323

Use forward slashes and quoted OCaml strings #323

Conversation

jonahbeckford commented Sep 19, 2023

Before

After

gasche left a comment

Choose a reason for hiding this comment

gasche commented Sep 20, 2023

jonahbeckford commented Sep 20, 2023

Test 1

jonahbeckford commented Sep 20, 2023 • edited Loading

Test 2 including OCaml quoted string literals

jonahbeckford commented Sep 20, 2023

Test 3 without quoted string literals

jonahbeckford commented Sep 20, 2023

gasche commented Sep 20, 2023

kit-ty-kate commented Sep 21, 2023

gasche commented Sep 21, 2023

kit-ty-kate commented Sep 21, 2023

gasche commented Sep 21, 2023

jonahbeckford commented Sep 22, 2023

hhugo commented Jun 20, 2024

jonahbeckford commented Jun 20, 2024

hhugo commented Jun 21, 2024

jonahbeckford commented Jun 21, 2024 • edited Loading

dra27 commented Jun 26, 2024

hhugo commented Jun 26, 2024

gasche commented Jun 26, 2024

jonahbeckford commented Sep 20, 2023 •

edited

Loading

jonahbeckford commented Jun 21, 2024 •

edited

Loading