Closed jonahbeckford closed 4 months ago
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?
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 contentOCAML_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
?
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.
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/someackspace\dir/bin"
let libdir = "/tmp/someackspace\dir/lib"
let ocaml_libdir = "/home/jonahbeckford/.opam/opam-publish/lib/ocaml"
let libdir_abs = "/tmp/someackspace\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: https://github.com/diskuv/dkml-installer-ocaml/issues/70.
That means backslash characters were already unsafe. Perhaps backslashed spaces work, but if it did that was an accident.
Anyway, I think this needs to be communicated as a major breaking change. ocamlbuild.1.0.0
?
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?
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 inocamlbuild -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
inecho 'let libdir_abs = {|$(abspath $(OCAMLBUILD_LIBDIR))|}';
, we get a nonsensical three-termlet 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?
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:
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).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
callsmake -f configure.make
, and I don't think it is allowed to assume thatocaml
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
)
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?!)
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 inocamlbuild -where
which will mess up some users of ocamlbuild:do you have an example of such users?
https://github.com/diskuv/dkml-installer-ocaml/issues/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.
@jonahbeckford would you be able to check if #334 works for your setup ?
@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!
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!).
Makefile.config
still contains backslashes, the quoted-strings part from this PR extracted in #329 means everything's fixed?I believe that everything here is fixed
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!
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:
src/ocamlbuild_config.ml:
After
Makefile.config:
src/ocamlbuild_config.ml: