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

use ocamlfind instead of opam to discover where ocaml-src is located #142

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Nov 22, 2024

and where to install and uninstall to

Discussed in #134 (comment) and #134 (comment) by @shym and @dinosaure

@shym
Copy link
Contributor

shym commented Nov 22, 2024

I wonder if using $(ocamlfind query ocaml-src) to find parent directories is not a bit fragile: if this is not in an OPAM-like setup, we could end up installing and removing stuff in wrong directories. I would be more comfortable for those scripts to error out if the prefix is not explicitly set.

@dinosaure
Copy link
Member

May be ocamlfind printconf destdir is preferable. However, prefix seems highly related to the OPAM layout.

@dinosaure
Copy link
Member

We should probably also just fail if $PREFIX is not set.

@hannesm
Copy link
Member Author

hannesm commented Nov 26, 2024

thanks for your feedback, I removed uninstall (it is unused anyways), and adapted install.sh to use $PREFIX.

@hannesm
Copy link
Member Author

hannesm commented Nov 27, 2024

the remaining question I have is how defensive should install.sh be? should we warn and fail on all potential issues (PREFIX being undefined, MAKE being undefined, ...)? Or is it fine to just use the empty prefix, an undefined MAKE, ...? Since it is called by make install (and there's no expectation to call it differently), I'm fine with the no error handling approach as proposed in this PR.

If there's some strong opinion to act differently, please raise your voice - best with a brief explanation why it should be more defensive.

Copy link
Contributor

@shym shym left a comment

Choose a reason for hiding this comment

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

This looks good to me. I agree with the approach: install.sh doesn’t need to accomodate for direct invocation and so $PREFIX should be empty only when the user has explicitly set it so in configure.sh, so there should be no surprise there.

@hannesm hannesm merged commit 057c1e2 into mirage:main Nov 27, 2024
6 checks passed
@hannesm hannesm deleted the use-ocamlfind branch November 27, 2024 17:49
@hannesm
Copy link
Member Author

hannesm commented Nov 27, 2024

Thanks, squash-merged.

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.

3 participants