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

Fix dune subst not to initialize root #4048

Merged
merged 1 commit into from
Jan 6, 2021

Conversation

rgrinberg
Copy link
Member

We explicitly read --debug-backtraces instead of relying on
Common.term. Common.term ended up bringing an entire kitchen sink of
stuff we don't want. For instance, root directory discovery.

We explicitly read --debug-backtraces instead of relying on
`Common.term`. `Common.term` ended up bringing an entire kitchen sink of
stuff we don't want. For instance, root directory discovery.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@ejgallego
Copy link
Collaborator

For completeness, this problem was introduced #3878 , it could make sense to have the init structure for different commands as an API in common.

@rgrinberg
Copy link
Member Author

it could make sense to have the init structure for different commands as an API in common.

Indeed it would. Though I don't see any other options that are relevant to $ dune subst at the moment.

My bad for suggesting to introduce this bug in the original PR in the first place.

@ejgallego
Copy link
Collaborator

I don't know all the utility commands so well, thus I'm not sure what for example dune install and dune subst have in common, at least there are usually called with by opam, right? I was thinking of something such as having Common.Build_command and Common.Pkg_command etc... so we can capture the invariants of the init context of commands better.

[I am not sure I care so much about the options, but indeed more about the init process on itself]

@rgrinberg
Copy link
Member Author

I don't know all the utility commands so well, thus I'm not sure what for example dune install and dune subst have in common, at least there are usually called with by opam, right?

dune install is not called by opam. Opam uses its own opam-installer binary and dune install is our own re-implementation of it.

so we can capture the invariants of the init context of commands better.

Yeah, that sounds good. I think we'll have a better idea once we add a few more commands unrelated to build.

Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

Looks good to me.

(mostly a revert of 68b32b6)

@rgrinberg rgrinberg merged commit 967719b into ocaml:master Jan 6, 2021
@rgrinberg rgrinberg deleted the subst-debug-backtrace branch January 6, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants