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.

Sign up for GitHub

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

Jump to bottom

Ensure installed stdlib artefacts have correct case #10301

Merged
merged 2 commits into from
Apr 20, 2021
Merged

Ensure installed stdlib artefacts have correct case #10301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61e830d
Ensure installed stdlib artefacts have correct case
dra27 Mar 19, 2021
e39bf9c
Abort install if stale artefacts found
dra27 Apr 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 6 additions & 5 deletions Changes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,12 @@ Working version
(Gabriel Scherer, review by Nicolás Ojeda Bär, Alain Frisch, Xavier Leroy,
Daniel Bünzli and Stephen Dolan)

* #10169, #10270: Use capitalized module names in the Standard Library prefixing
scheme to match Dune, e.g. Stdlib__String instead of Stdlib__string. This is a
breaking change only to code which attempted to use the internal names before.
The Standard Library generated by the Dune rules is now equivalent to the main
build (the Dune rules still do not generate a distributable compiler).
* #10169, #10270, #10301: Use capitalized module names in the Standard Library
prefixing scheme to match Dune, e.g. Stdlib__String instead of Stdlib__string.
This is a breaking change only to code which attempted to use the internal
names before. The Standard Library generated by the Dune rules is now
equivalent to the main build (the Dune rules still do not generate a
distributable compiler).
(David Allsopp and Mark Shinwell, review by Gabriel Scherer)

### Other libraries:
Expand Down
22 changes: 21 additions & 1 deletion stdlib/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ allopt: stdlib.cmxa std_exit.cmx
opt.opt: allopt

.PHONY: install
# Ensure any pre-4.13 lowercased artefacts are removed on macOS and Windows
install::
stale="$(filter-out $(notdir $(wildcard stdlib__*.cmi)), \
$(notdir $(wildcard $(INSTALL_LIBDIR)/stdlib__*.cmi)))"; \
if test -n "$$stale" ; then \
echo "$(INSTALL_LIBDIR) contains stale stdlib artefacts"; \
echo "Please rm $(INSTALL_LIBDIR)/stdlib__*.cm* and re-run make install"; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo "Please rm $(INSTALL_LIBDIR)/stdlib__*.cm* and re-run make install"; \
echo "Please rm $(INSTALL_LIBDIR)/stdlib__*.cmi and re-run make install"; \

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well noticed 🙂 My theory here was that it would be annoying to be told at this stage to delete the .cmi files only to be told when you rerun make install that you should now also delete the .cmx files. The other message can only be reached (officially) if the .cmi files had already been deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

If cmt and cmti are already present can this cause issue ? The variable stale only check for cmi.

Copy link
Member Author

Choose a reason for hiding this comment

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

You'd need to have installed 4.07-4.12, manually removed only the .cmi files and then also be in the relatively unusual position of wanting to install 4.13+ over the top of that manually-broken old installation 🙂

Source artefacts are installed in the install:: rule following this one - there could be the same check there, but it feels somewhat overkill to add it?

exit 1; \
fi

install::
$(INSTALL_DATA) \
stdlib.cma std_exit.cmo *.cmi camlheader_ur \
Expand All @@ -81,7 +91,17 @@ endif
installopt: installopt-default

.PHONY: installopt-default
installopt-default:
# Ensure any pre-4.13 lowercased artefacts are removed on macOS and Windows
installopt-default::
stale="$(filter-out $(notdir $(wildcard stdlib__*.cmx)), \
$(notdir $(wildcard $(INSTALL_LIBDIR)/stdlib__*.cmx)))"; \
if test -n "$$stale" ; then \
echo "$(INSTALL_LIBDIR) contains stale stdlib artefacts"; \
echo "Please rm $(INSTALL_LIBDIR)/stdlib__*.cmx and re-run make install"; \
exit 1; \
fi

installopt-default::
$(INSTALL_DATA) \
stdlib.cmxa stdlib.$(A) std_exit.$(O) *.cmx \
"$(INSTALL_LIBDIR)"
Expand Down