Clear error when the ocaml working directory path contains spaces

[Bibi210]

Hello, I'm a student in IT.
I would like to make my first pull request to an open project.

As discussed with @gasche we discovered that the dev testsuite doesn't handle spaces in the ocaml working directory path.

While building and setting up,
I've noticed that i wasn't able to use the dev testsuite.

This is what happens :

bimac@BiMac ocaml % make tests
gmake -C testsuite all
gmake[1]: Entering directory '/Users/bimac/Licence_3/Log Libre/ocaml/testsuite'
gmake[3]: *** No rule to make target 'Libre/ocaml/testsuite'.  Stop.
gmake[2]: *** [Makefile:339: tools] Error 2
gmake[1]: *** [Makefile:166: all] Error 2
gmake[1]: Leaving directory '/Users/bimac/Licence_3/Log Libre/ocaml/testsuite'
gmake: *** [Makefile:611: tests] Error 2

Since it is common to use spaces on file directory,
I've tried to work on the testsuite makefile, but the issue also take root further away in ocamltest

Until the problem is solved I propose a pull request in order to generate a clean error message, so that others newcomers can find out quickly the issue

[Bibi210]

Is there something else I should do to help reviewing this PR ?
@shindere @Octachron

[shindere]

Welcome on board @Bibi210 and many thanks for having chosen OCaml as the first project where you contribute. I wish your experience as a contributor will be rich, positive and rewarding. @dra27 may also have an opinion about this PR. It looks fine to me, thus I am going to approve it. It could be helpful to create an issue to track the fact that ocamltest does not handle spaces in dirnames properly, with a bit of details if possible: show the output you get when running ocmaltest on a test whose dirname ocntains a space. If you create such an issue, please mention me to make sure I am notified. Still, and although spaces in paths are common on Windows, it's something `make` in particular and the GNU tools in general do not deal well with. I mean, they don't, and they don't intend to, so fighting for that may be a frustrating batle at some point. Not to discourage you but just so that you are warned.

[nojb]

As @shindere said, there is not much we can do about spaces in Makefiles. I wonder if it is worth adding this check just for the testsuite, as the same problem is probably present in other Makefiles/variables. Personally, I would leave things as they are, since anyway any "fix" will at most be partial, and quick "fixes" can introduce bugs of their own (cf the comment with the double quotes).

Basically anyone using Makefiles should know that spaces in directory names are a "no-no".

[nojb]

This command shells out two processes each time the Makefile is invoked, which can be a bit expensive. Instead, you can use pure make:

Suggested change

	ifneq "$(shell echo $(BASEDIR) | grep ' ')" ""
	ifneq "$(words |$(BASEDIR)|)" "1"

[Bibi210]

Thanks for the change I'm quite new with makefiles.

[nojb]

These quotes make me a bit nervous: the variable is also sorrounded by quotes also below (in the definition of BASEDIR_HOST and maybe in other places). Won't that cause problems?

[Bibi210]

I will remove them since they are no longer needed

[gasche]

Most of the compiler-distribution Makefiles only use relative paths to refer to build artifacts (so: no spaces as we control the directory names); the only absolute paths are for the INSTALL_* directories, and those are carefully quoted. The testsuite Makefile and the ocamltest codebase itself are specific in that they use absolute directories during development.

I tried to make ocamltest work with spaces in the current-working-directory path, and it's a huge amount of work (I got #11111, #11112 out of it as nice independent fixes); I suspect that it's a dead-end and that the current proposal (to have a clean error message instead of a confusing error) is better in practice.

[nojb]

the ocamltest codebase itself are specific in that they use absolute directories during development.

Fair point, I realised this after submitting my review. Am OK with including this change, but the issue of the repeated double quotes still needs to be looked at I think.

[gasche]

Sure. (I was also surprised, but it works in my testing on Linux, with or without spaces in the current path.) It may not be necessary anymore with your nice make-expert proposal.

[shindere]

Yes, I tend to agree with your overall analysis, @nojb. Perhaps one way to go would be to make sure we only deal with relative paths, but given how make odes not support spaces in directory names, I find having to hack it to support them is a pain. If I could, I'd even remove the existing support and live with the assumption that directory names never contain spaces. Would make a few things simpler especially for installation related targets.

[gasche]

I pushed my work-in-progress experiments to a branch testsuite-makefile-space of my online fork:
https://github.com/gasche/ocaml/tree/testsuite-makefile-space
https://github.com/gasche/ocaml/commits/testsuite-makefile-space

In this branch, fixing the ocamltest Makefile to work well with spaces in the cwd path was fairly easy:
gasche@cdd3d0c
on the other hand, my attempt to fix ocamltest itself to deal nicely with spaces in path was an invasive mess
gasche@b32b863

I eventually got to the point where basic invocation of ocamltest works, about half the tests work (the simple ones), and another half fail for various failure reasons that would be investigated one by one.

Maybe there are less-invasive ways to update the ocamltest codebase. (Currently the codebase builds the command-line as a string, then splits it again at exec time; I tried to keep the arguments separate, which is conceptually the right approach but is a lot of work, and also still requires splitting the string content of the ocamltest variables). But in any case, I think that part would be more work than fixing the Makefile itself which isn't so hard in practice.

[Bibi210]

Welcome on board @Bibi210 and many thanks for having chosen OCaml as the first project where you contribute. I wish your experience as a contributor will be rich, positive and rewarding. @dra27 may also have an opinion about this PR. It looks fine to me, thus I am going to approve it. It could be helpful to create an issue to track the fact that ocamltest does not handle spaces in dirnames properly, with a bit of details if possible: show the output you get when running ocmaltest on a test whose dirname ocntains a space. If you create such an issue, please mention me to make sure I am notified. Still, and although spaces in paths are common on Windows, it's something make in particular and the GNU tools in general do not deal well with. I mean, they don't, and they don't intend to, so fighting for that may be a frustrating batle at some point. Not to discourage you but just so that you are warned.

Thanks, I will try to open the issue as soon as possible
I also removed the unneeded quotes.

[Et7f3]

I tried to solve this issue in #10727 but seem that @gasche didn't see it.

[gasche]

Yes, I had missed it. Maybe you should consider pinging the people there who started reviewing?

(I think there is a bit too much (impressive!) work in your PR, and you should consider splitting in several PRs touching independent parts of the codebase. For example the how-to-generate-the-shebang fix can go on its own, the ocamltest-related changes on their own, etc. Currently it's hard for people to review and discuss all at once, so nobody does it.)