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.

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

Provide findlib-install target #6

Merged
merged 1 commit into from
Jan 11, 2018
Merged

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jan 10, 2018

See also discussions in ocaml/opam-repository#10857 and #5.

This is an adaptation of #5 providing a separate findlib-install target which the opam num package can use for system switches.

Not installing things in /usr/local/lib/ocaml in an opam system switch is of course an opam packaging concern, but it seems better to me to control the process here in num's repository and have it that the packaging concern is running make findlib-install if appropriate.

In future, we could change the install target to be the same as findlib-install.

This patch is presently being applied to 1.0 and 1.1 in ocaml/opam-repository#11207

Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

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

Thanks for this suggestion, I like it very much.

Some suggested updates below, plus one:

  • Add one sentence to the comment at the top of file META.in to say "If the directory line is removed, it installs in a subdirectory, as normal for a findlib package."

findlib-install:
$(MAKE) -C src findlib-install
$(MAKE) -C toplevel install

uninstall:
$(MAKE) -C src uninstall
$(MAKE) -C toplevel uninstall

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a findlib-uninstall entry for symmetry.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

src/Makefile Outdated
META.findlib: META.in
rm -f META.legacy
fgrep -v directory $^ > META
touch $@
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen fgrep used in ages :-) It's not part of POSIX (IEEE Std 1003.1 2004) so please just use grep.

Copy link
Member Author

Choose a reason for hiding this comment

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

😄 I didn't know I was so old-fashioned, though is anyone surprised?!

src/Makefile Outdated

install:
META.legacy: META.in
rm -f META.findlib
Copy link
Contributor

Choose a reason for hiding this comment

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

The two META.legacy and META.findlib entries are hard to understand. I think you can simplify by removing those two entries and "inlining" the construction of META from META.in in the install and findlib-install entries, see below.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK - my personal Makefile "taste" (if you can call it that) is that even cping a file is a compilation step, so it's a dependency, but I've changed it!

src/Makefile Outdated
fgrep -v directory $^ > META
touch $@

install: META.legacy
$(OCAMLFIND) install num META
Copy link
Contributor

Choose a reason for hiding this comment

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

After "inlining" this would be

install:
    cp META.in META
   ...
   rm -f META

src/Makefile Outdated
$(OCAMLFIND) install num META
$(INSTALL_DATA) $(TOINSTALL) $(STDLIBDIR)
ifeq "$(SUPPORTS_SHARED_LIBRARIES)" "true"
$(INSTALL_DIR) $(STDLIBDIR)/stublibs
$(INSTALL_DLL) $(TOINSTALL_STUBS) $(STDLIBDIR)/stublibs
endif

findlib-install: META.findlib
$(OCAMLFIND) install num META $(TOINSTALL) $(TOINSTALL_STUBS)
Copy link
Contributor

Choose a reason for hiding this comment

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

After "inlining" this would be:

findlib-install:
    grep -v 'directory' META.in > META
    $(OCAMLFIND) install ...
    rm -f META

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, for neatness, I've changed the grep to grep -Fv '^' as that exactly removes the directory instruction and not also various lines of the comment.

src/Makefile Outdated
$(OCAMLFIND) install num META
$(INSTALL_DATA) $(TOINSTALL) $(STDLIBDIR)
ifeq "$(SUPPORTS_SHARED_LIBRARIES)" "true"
$(INSTALL_DIR) $(STDLIBDIR)/stublibs
$(INSTALL_DLL) $(TOINSTALL_STUBS) $(STDLIBDIR)/stublibs
endif

findlib-install: META.findlib
$(OCAMLFIND) install num META $(TOINSTALL) $(TOINSTALL_STUBS)

uninstall:
cd $(STDLIBDIR) && rm -f $(TOINSTALL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add findlib-uninstall entry for symmetry.

Copy link
Member Author

Choose a reason for hiding this comment

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

It irrelevantly changes the order of the uninstall, but I've made findlib-uninstall a "dependency" of uninstall, as that's actually the same command.

Allows installing the entire library using ocamlfind.
@dra27
Copy link
Member Author

dra27 commented Jan 10, 2018

Thanks Xavier - all points addressed, I think. I changed the sense of the sentence in the META file, as META files describe what has happened rather than what will happen.

@xavierleroy xavierleroy merged commit 7dd5e93 into ocaml:master Jan 11, 2018
yakobowski added a commit to yakobowski/opam-repository-mingw that referenced this pull request May 14, 2019
Need to ocaml/opam-repository#10857, which
is solved by ocaml/num#6 (but this patch is not
merged in num 1.1)
yakobowski added a commit to yakobowski/opam-repository-mingw that referenced this pull request May 28, 2019
Need to ocaml/opam-repository#10857, which
is solved by ocaml/num#6 (but this patch is not
merged in num 1.1)

Also fix syntax for 'preinstalled' flag, which has evolved since the PR
on the ocaml/num repo.
@yakobowski
Copy link

@xavierleroy would you consider making a new release of Num, which would contain in particular this PR? The changes have been propagated to the Opam repo as a patch, but not to the Windows one (which admittedly is not supposed to use a ocaml-system compiler). As a result, I have been bitten by this issue. Also, this also mean that you cannot easily pin opam pin num, should you wish to do so.

@xavierleroy
Copy link
Contributor

Release 1.2 is here: https://github.com/ocaml/num/releases/tag/v1.2 . Enjoy!

@dra27
Copy link
Member Author

dra27 commented Jun 21, 2019

@xavierleroy - are you going to do a package release to opam-repository, or would you like me to?

@dra27 dra27 deleted the install-findlib branch June 21, 2019 10:46
@xavierleroy
Copy link
Contributor

I submitted a tentative OPAM package: ocaml/opam-repository#14331 . I'd be happy if you could review it and make sure I didn't make mistakes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants