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

Inspect OCAMLPATH when not using ocamlfind #642

Merged
3 commits merged into from Mar 20, 2018
Merged

Inspect OCAMLPATH when not using ocamlfind #642

3 commits merged into from Mar 20, 2018

Conversation

ghost
Copy link

@ghost ghost commented Mar 20, 2018

No description provided.

@ghost ghost changed the title Inspect ocamlpath when not using ocamlfind Inspect OCAMLPATH when not using ocamlfind Mar 20, 2018
@ghost ghost requested a review from rgrinberg March 20, 2018 15:31
rgrinberg
rgrinberg reviewed Mar 20, 2018
View reviewed changes
src/context.ml
| None , None -> None
| Some s, None -> Some s
| None , Some _ -> None
| Some x, Some y -> Option.some_if (x <> y) x
Copy link
Member

Choose a reason for hiding this comment

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

Why do we discard the OCAMLPATH if it's set in the context? Is it because we're afraid it's simply set by the parent process? I think in this case, opam should simply unset when giving us the environment variables.

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I guess opam should do it indeed, but currently it doesn't. Additionally, for cross-compilation we don't rely on opam.

rgrinberg
rgrinberg approved these changes Mar 20, 2018
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

OK, sure. A couple of things to consider regarding the opam + OCAMLPATH issue:

  • Perhaps we should print a warning?
  • Is there an opam bug report for this issue?

@rgrinberg
Copy link
Member

Btw, master already contains the CHANGELOG entry (with pr number + attribution) so you can remove it here.

@ghost
Copy link
Author

ghost commented Mar 20, 2018

Perhaps we should print a warning?

I don't think so, there is no easy way for users to disable the warning appart from completely unsetting the variable

Is there an opam bug report for this issue?

It seems like it, opam should probably always set this variable. But Dune should do it as well, for instance for x-compilation.

@ghost ghost merged commit f5f151d into ocaml:master Mar 20, 2018
rgrinberg pushed a commit that referenced this pull request Mar 20, 2018
@jeremiedimino @rgrinberg
Inspect OCAMLPATH when not using ocamlfind (#642)
150df31
rgrinberg pushed a commit that referenced this pull request Mar 20, 2018
@jeremiedimino @rgrinberg
Inspect OCAMLPATH when not using ocamlfind (#642)
6496038
rgrinberg pushed a commit that referenced this pull request Mar 20, 2018
@jeremiedimino @rgrinberg
Inspect OCAMLPATH when not using ocamlfind (#642)
4c4e40f
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rgrinberg @jeremiedimino