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

Upstream Jane Street changes to Dune engine #8362

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

dkalinichenko-js
Copy link
Collaborator

@dkalinichenko-js dkalinichenko-js commented Aug 10, 2023

+ minor changes to Stdune and Dune rules

Copy link
Collaborator

@Alizter Alizter left a comment

Choose a reason for hiding this comment

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

Do we want to expose the stop_on_first_error behaviour to the user via a config option?

@@ -96,13 +96,15 @@ type t =
; execution_parameters : dir:Path.Source.t -> Execution_parameters.t Memo.t
; source_tree : (module Source_tree)
; action_runner : Action_exec.input -> Action_runner.t option
; action_runners : unit -> Action_runner.t list
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the difference between this and the field above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

First one is for selecting the appropriate action runner based on the actions, second one is for notifying all action runners. Merging those is non-trivial since selection depends on some JS-specific code. We plan to refactor this but for now we're trying to minimize the diff between our version and upstream.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very well. Can you include a comment in the mli that says this will be refactored at a later date? Descriptions of the fields would also be nice but not necessary.

@rgrinberg
Copy link
Member

As discussed offline: the test suite reveals that our cycles errors aren't marshallable. We should be able to fix that.

I've glanced over the code and there's a couple of things I've noticed that are missing:

  1. Stdout/Stderr truncation introduced by Jesse
  2. The recording of action duration in Action_exec

I'm not sure if that was intentional, but it's fine to introduce these changes in subsequent PR's if you prefer.

@dkalinichenko-js
Copy link
Collaborator Author

Do we want to expose the stop_on_first_error behaviour to the user via a config option?

That would be nice, we implemented that at JS but our front-end is different.

@dkalinichenko-js
Copy link
Collaborator Author

dkalinichenko-js commented Aug 10, 2023

I've glanced over the code and there's a couple of things I've noticed that are missing:

  1. Stdout/Stderr truncation introduced by Jesse
  2. The recording of action duration in Action_exec

I'm not sure if that was intentional, but it's fine to introduce these changes in subsequent PR's if you prefer.

Yeah, both are intentional, I'll do it in later PRs.

@@ -96,13 +96,15 @@ type t =
; execution_parameters : dir:Path.Source.t -> Execution_parameters.t Memo.t
; source_tree : (module Source_tree)
; action_runner : Action_exec.input -> Action_runner.t option
; action_runners : unit -> Action_runner.t list
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very well. Can you include a comment in the mli that says this will be refactored at a later date? Descriptions of the fields would also be nice but not necessary.

@Alizter
Copy link
Collaborator

Alizter commented Aug 10, 2023

Do we want to expose the stop_on_first_error behaviour to the user via a config option?

That would be nice, we implemented that at JS but our front-end is different.

OK, once this PR is merged we should open an issue about adding this feature in. We can discuss the details in the issue.

Signed-off-by: Diana Kalinichenko <dkalinichenko@janestreet.com>
Signed-off-by: Diana Kalinichenko <dkalinichenko@janestreet.com>
@dkalinichenko-js
Copy link
Collaborator Author

Looks like it's not just cycle errors that can be raised in Action_exec with dynamic dependencies.

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

Alizter commented Aug 11, 2023

Looks like coveralls is getting confused by this branch being named main and also reporting the coverage here as mains coverage. I'll let them know upstream.

_
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
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.

4 participants