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

Windows: Use common path for standard library #2004

Merged
merged 2 commits into from Aug 28, 2018

Conversation

Projects
None yet
4 participants
@bryphe
Copy link
Contributor

commented Aug 23, 2018

First off, thank you for all the incredible work you've done with the OCaml compiler!

I've been experimenting with the OCaml compiler as part of the https://github.com/esy/esy project, in particular, facilitating a smooth Windows experience.

I noticed that the standard library path was different on Windows vs OSX / Linux:

  • Windows: <ocaml>/lib
  • OSX / Linux: <ocaml>/lib/ocaml

It seems like it would make sense for these to be consistent cross-platform. Looking at the history of the Makefiles, it looks like the LIBDIR was set a while ago - so I'm curious if perhaps it was changed in OSX / Linux, but not on Windows. For most tools and applications, it may not be important, as the OCAMLLIB variable can be used - but esy is a somewhat special case as it implements a sandbox isolation model, and needs strict control over the environment. Having a common path from the ocaml root means less special casing per-platform.

In addition, I wonder if there are any compatibility issues making this change. Please let me know if you'd like any information or have any feedback on this change. Thank you!

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

In principle this seems a harmless change: Windows users typically modify their Makefiles anyway before building. But let's see what the build system experts think about it.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

Well, not being a user of Windows I have not strong opinion on this.
Perhaps @dra27 wants to say something?

@dra27

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

In opam, this is already done (see here) because it's such a nuisance having the layout being different.

I can't see how this would cause a problem - please add a Changes entry, though.

@dra27

dra27 approved these changes Aug 24, 2018

@bryphe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

Wonderful! Thanks all for the feedback, and thank you @dra27 for the review.

I've added a Changes entry (I added your name too @jordwalke since your suggestion prompted this change). Please let me know if you'd like any additional revisions.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

Please squash "Resolve merge conflicts" onto your first commit and we can merge this.

bryphe added some commits Aug 23, 2018

@bryphe bryphe force-pushed the bryphe:bryphe/windows/common-stdlib-path branch from 604ec2b to 356f53b Aug 28, 2018

@bryphe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2018

Done! Thanks @nojb .

@nojb nojb merged commit 9a91bf5 into ocaml:trunk Aug 28, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@bryphe bryphe deleted the bryphe:bryphe/windows/common-stdlib-path branch Aug 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.