Allow installing in folder with space in name by Et7f3 · Pull Request #11590 · ocaml/ocaml

Et7f3 · 2022-10-01T01:05:04Z

Hello,

As part of split of #10727 here a first step.
https://github.com/ocaml/ocaml/pull/10727/files#r748650610 comment associated.

This allow to install man page and be able to see it:

./configure --prefix="$PWD/install here"
make
make install
export MANPATH="$PWD/install here"/man
man Lazy # We get the recent version

check-stale allow finer message. So it it a improvement on itself and it works with space in path.

shindere · 2022-10-03T10:11:08Z

This seems correct to me, but I'd like @dra27's review, please.

dra27

The fix to man/Makefile is clearly correct (thanks!)

Other changes we've been making have been trying to move away from using shell constructs and the dancing with bash-isms, not to mention having to worry about locales at all, makes me very nervous about the other change. We have been stung - at considerable loss of time both to the original reporter and to those who had to reproduce and fix it - by the vagaries of shell commands being sensitive to user locales, and GNU make (like ISO C!) has the benefit of being evaluated in the C locale.

The test can fairly easily be kept in make, just by taking advantage of make -C <dir> -f <Makefile> to do the change of directory. I tested the idea in dra27@83b1238 which works both on Windows and macOS (I installing this over the top of a 4.12 installation). The crux of it is that the install:: target becomes:

.PHONY: install
# Ensure any pre-4.13 lowercased artefacts are removed on macOS and Windows
install::
	@$(MAKE) -C '$(INSTALL_LIBDIR)' -f '$(realpath Makefile.check-stale)' \
	         EXT=.cmi \
	         'ARTEFACTS=$(wildcard stdlib__*.cmi)' \
	         'INSTALL_LIBDIR=$(INSTALL_LIBDIR)'

which makes a recursive call in the target directory and the GNU make definition for $(stale) is expanded into MAkefile.check-stale as:

INSTALLED_ARTEFACTS = $(notdir $(wildcard stdlib__*$(EXT)))
STALE_ARTEFACTS = $(filter-out $(ARTEFACTS), $(INSTALLED_ARTEFACTS))
endif

and now $(STALE_ARTEFACTS) will be non-empty if any files with the old casing are found. The "trick" is that using -C avoids the need for specifying paths with spaces in them inside a Makefile (which is impossible)

Changes

Et7f3 · 2022-10-03T21:08:25Z

Ok so you want I commit your code with you as co-author ?

dra27 · 2022-10-04T08:38:25Z

If you're happy with that commit, sure - the main thing is that I think the check should be done using GNU make rather than using the shell, I just didn't want to suggest that before being sure that the solution actually worked (which is why I wrote the commit!)

Et7f3 · 2022-10-04T09:03:27Z

EXT=.cmi instead of .cm* is it for a reason ? Inlining it in the wildcard should be fine ?

dra27 · 2022-10-04T11:22:28Z

EXT=.cmi instead of .cm* is it for a reason ? Inlining it in the wildcard should be fine ?

As in https://github.com/ocaml/ocaml/pull/10301/files#r746338143 🙂

Et7f3 · 2022-10-04T11:48:56Z

A yes. Thx for reminder (it has been more than one year).

Co-authored-by: David Allsopp <david.allsopp@metastack.com> Signed-off-by: Et7f3 <cadeaudeelie@gmail.com>

Et7f3 · 2022-10-06T11:55:05Z

Since installopt-default and install depend on this staleness check I keep the auxiliary target. I removed echo bashim with makefile error command and if to check if the variable is empty.

A HACKTOBERFEST-ACCEPTED label is appreciated if you consider this PR non-spammy.

dra27 · 2022-10-13T11:58:30Z

Sorry for the delay getting back to you, @Et7f3 - there's consensus that we can take an even simpler route with stdlib/Makefile and just delete the check for mis-cased artefacts completely! Would you be happy to make that change part of this PR? (it just means deleting entirely LL57-67, re-adding the .PHONY: install above L70 and deleting LL97-106)

Et7f3 · 2022-10-13T13:34:09Z

CI is green

dra27 · 2022-10-13T13:57:50Z

Changes


 ### Build system:
- #11590: Allow installing to a destination path containing spaces.
+- #11590: Delete the check for mis-cased artefacts


I don't think change is necessary - the PR still primarily allows installation to a destination path containing spaces!

Ok old commit 6ed82b5 I just reverted Changes

dra27

Good to go with CI, thanks!

Thanks for your patience with this one - I’m reasonably confident that the next ones won’t take quite so much “to and fro” 🙂

kit-ty-kate · 2023-01-03T16:08:59Z

Is there any chance this could be backported to the 4.14 and 5.0 branches?

dra27 · 2023-01-03T16:35:55Z

Certainly fine for the man/Makefile change - not instantly sure about the stdlib/Makefile change (it removes the test rather than fixing it)

Et7f3 · 2023-01-03T23:04:46Z

We have found a version that test and doesn't fail with space in path. Do you want I restore it ?

dra27 · 2023-01-04T15:11:17Z

No, it's OK, thanks - we certainly won't back-port a fix we didn't merge! @Octachron - can I manually push the man/Makefile change to 4.14+5.0 (possibly further, for cases where that's the only blocking issue for installing to directories with spaces)

Octachron · 2023-01-04T15:21:16Z

The man changes seems very fine to backport directly to 4.14 and 5.0 to me.

- fix(build): Repair make install (in man/Makefile)

dra27 · 2023-01-04T16:35:26Z

Pushed to both, thanks!

- fix(build): Repair make install (in man/Makefile)

Et7f3 changed the title ~~fix(build): Repair make install~~ Allow installing in folder with space in name Oct 1, 2022

fix(build): Repair make install

cce0f57

Et7f3 force-pushed the Allow-install-path-with-space branch from 4f5dc36 to cce0f57 Compare October 1, 2022 01:08

dra27 self-assigned this Oct 1, 2022

dra27 self-requested a review October 1, 2022 07:28

dra27 reviewed Oct 3, 2022

View reviewed changes

Changes Outdated Show resolved Hide resolved

Remove bashism from Makefile

de47c4f

Co-authored-by: David Allsopp <david.allsopp@metastack.com> Signed-off-by: Et7f3 <cadeaudeelie@gmail.com>

dra27 reviewed Oct 13, 2022

View reviewed changes

Delete the check for mis-cased artefacts

f459b70

Et7f3 force-pushed the Allow-install-path-with-space branch from 6ed82b5 to f459b70 Compare October 13, 2022 14:02

dra27 approved these changes Oct 13, 2022

View reviewed changes

dra27 merged commit 9341e40 into ocaml:trunk Oct 18, 2022

Et7f3 deleted the Allow-install-path-with-space branch October 21, 2022 00:17

kit-ty-kate mentioned this pull request Jan 3, 2023

Add spaces to opam root during CI ocurrent/opam-repo-ci#199

Open

kit-ty-kate mentioned this pull request Jan 3, 2023

Add a check using a local switch with spaces in the directory name ocurrent/opam-repo-ci#200

Draft

dra27 added a commit that referenced this pull request Jan 4, 2023

Allow installing in folder with space in name (#11590)

a3cd42b

- fix(build): Repair make install (in man/Makefile)

dra27 added a commit that referenced this pull request Jan 4, 2023

Allow installing in folder with space in name (#11590)

74fe398

- fix(build): Repair make install (in man/Makefile)

voodoos pushed a commit to voodoos/ocaml that referenced this pull request Feb 10, 2023

Allow installing in folder with space in name (ocaml#11590)

1a13082

- fix(build): Repair make install (in man/Makefile)

kit-ty-kate mentioned this pull request Oct 9, 2023

Opam fails to install in directory with spaces in the name. ocaml/opam#5679

Closed

Khady mentioned this pull request Nov 1, 2023

compiling ocaml in a folder with space in its name creates a binary with an invalid shebang #12709

Open

ejgallego pushed a commit to ejgallego/ocaml that referenced this pull request Nov 5, 2025

Allow installing in folder with space in name (ocaml#11590)

4acadfb

- fix(build): Repair make install (in man/Makefile)

Conversation

Et7f3 commented Oct 1, 2022

Uh oh!

shindere commented Oct 3, 2022 via email

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Et7f3 commented Oct 3, 2022

Uh oh!

dra27 commented Oct 4, 2022

Uh oh!

Et7f3 commented Oct 4, 2022

Uh oh!

dra27 commented Oct 4, 2022

Uh oh!

Et7f3 commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Et7f3 commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dra27 commented Oct 13, 2022

Uh oh!

Et7f3 commented Oct 13, 2022

Uh oh!

dra27 Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

Et7f3 Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate commented Jan 3, 2023

Uh oh!

dra27 commented Jan 3, 2023

Uh oh!

Et7f3 commented Jan 3, 2023

Uh oh!

dra27 commented Jan 4, 2023

Uh oh!

Octachron commented Jan 4, 2023

Uh oh!

dra27 commented Jan 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Et7f3 commented Oct 4, 2022 •

edited

Loading

Et7f3 commented Oct 6, 2022 •

edited

Loading