Skip to content

Remove result dependency#1293

Closed
rgrinberg wants to merge 1 commit intoocaml:masterfrom
rgrinberg:remove-result
Closed

Remove result dependency#1293
rgrinberg wants to merge 1 commit intoocaml:masterfrom
rgrinberg:remove-result

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg commented Mar 29, 2021

  • It's no longer necessary because merlin doesn't support 4.02.
  • This requires us to drop (implicit_transitive_deps false) temporarily.
    The reason is that Csexp is still using result.

@rgrinberg
Remove result dependency
410d165 
* It's no longer necessary because merlin doesn't support 4.02.
* This requires us to drop (implicit_transitive_deps false) temporarily.
The reason is that Csexp is still using result.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg requested review from trefis and voodoos March 29, 2021 06:17
@voodoos
Copy link
Copy Markdown
Collaborator

voodoos commented Mar 29, 2021

Maybe we should wait for a release of Csexp ?

@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Mar 29, 2021

Maybe we should wait for a release of Csexp ?

Then we'll need to add a lower bound on Csexp. Is that ok with you?

@trefis
Copy link
Copy Markdown
Contributor

trefis commented Mar 31, 2021

The reason is that Csexp is still using result.

At first I thought "But isn't Result.result defined as equal to result on all the versions we care about?", and then I realized the answer was "yes, but you can't know that without looking at its cmi!".

I think I'd rather keep (implicit_transitive_deps false) and the dependency on result for now.
Is there any reason to get rid of it?

@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Mar 31, 2021

I think I'd rather keep (implicit_transitive_deps false) and the dependency on result for now.
Is there any reason to get rid of it?

It makes (implicit_transitive_deps false) less usable for everyone downstream. For example, if I want to use Csexp and (implicit_transitive_deps false), I need to be able to interpret the issue you pointed out and add result as a dependency. People find this non obvious.

We can keep (implicit_transitive_deps false) if we bump the lower bound on csexp.

@trefis
Copy link
Copy Markdown
Contributor

trefis commented Apr 1, 2021

But, how does that concern us? We already have the right dependencies.

@voodoos voodoos mentioned this pull request Mar 18, 2022
Merged
@voodoos
Copy link
Copy Markdown
Collaborator

voodoos commented Mar 30, 2022

Result dependency was removed in #1441.

@voodoos voodoos closed this Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@voodoos voodoos Awaiting requested review from voodoos

@trefis trefis Awaiting requested review from trefis

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rgrinberg @voodoos @trefis