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

[fud2] Implement Shell Commands With Dependencies #2224

Merged
merged 35 commits into from
Aug 6, 2024
Merged

Conversation

jku20
Copy link
Collaborator

@jku20 jku20 commented Jul 25, 2024

Implements shell commands with dependencies. This has been split off from the origin PR for ease of review.

@jku20 jku20 marked this pull request as ready for review July 26, 2024 05:15
@jku20 jku20 requested a review from sampsyo July 26, 2024 05:23
@jku20
Copy link
Collaborator Author

jku20 commented Jul 29, 2024

oops, my git is bad, that didn't quite do what I thought it would

Copy link
Contributor

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Neato! Thanks for separating this out so it was easier to review; it was a lot easier to grok this after understanding #2203.

As you'll see, my big comment is that I don't think that the benefit of allowing mixtures of shell and shell_deps is worth it. It makes the logic pretty weird, it will make the Ninja harder to understand, and it doesn't seem like a feature people will actually want. I think we could trim back the conceptual complexity here if each op had to choose between the two modes of specifying commands.

fud2/fud-core/src/run.rs Outdated Show resolved Hide resolved
fud2/fud-core/src/run.rs Outdated Show resolved Hide resolved
fud2/fud-core/src/run.rs Show resolved Hide resolved
fud2/fud-core/src/run.rs Outdated Show resolved Hide resolved
fud2/fud-core/src/run.rs Outdated Show resolved Hide resolved
fud2/fud-core/src/run.rs Outdated Show resolved Hide resolved
@jku20
Copy link
Collaborator Author

jku20 commented Aug 5, 2024

The previous commit separates out shell and shell_deps functions, making it an error to use both in the same op. This allows for simpler logic when for emitting a RulesOp but requires more complicated logic when building the RhaiOp.

In particular, this implementation tags RhaiOp's cmd field with either being commands from shell, shell_deps, or being empty.

Base automatically changed from fud2-rhai-rework to main August 5, 2024 15:31
Copy link
Contributor

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Awesome! This all looks great as well.

fud2/fud-core/src/run.rs Outdated Show resolved Hide resolved
@rachitnigam rachitnigam added the C: fud2 experimental driver label Aug 5, 2024
@jku20 jku20 merged commit 0d578c7 into main Aug 6, 2024
18 checks passed
@jku20 jku20 deleted the fud2-generalized-shell branch August 6, 2024 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: fud2 experimental driver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants