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

tasks: pickup outgoing task dependencies #1573

Merged
merged 13 commits into from
Nov 25, 2024
Merged

tasks: pickup outgoing task dependencies #1573

merged 13 commits into from
Nov 25, 2024

Conversation

sandydoo
Copy link
Member

@sandydoo sandydoo commented Nov 4, 2024

I think we're running the tasks in reverse order after the toposort. The following tasks are speced to run after enterShell, but they instead run before.

This PR lets the graph traversal visit both incoming and outgoing edges.
Traversing all the edges of a node allows picking up tasks that are only specified to run after the current node. For example:

tasks."my-app:after-venv" = {
  exec = "echo hello";
  after = [ "devenv:python:virtualenv" ];
};

Previously, the only way to do this was to also use before to add this dependency to the graph, usually via devenv:enterShell. This change also allows running tasks after devenv:enterShell.

Fixes #1557.

TODO

While this change makes setting up tasks more predictable, we'll need to make this behavior configurable to support one-off tasks. You might want to run just the specified task, or all the tasks up to the specified task.

This also opens up the question of different types of constraints, e.g. ordering vs dependency. systemd has After and Requires.

  • Update existing tests

Copy link

cloudflare-workers-and-pages bot commented Nov 5, 2024

Deploying devenv with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0b6d9ee
Status: ✅  Deploy successful!
Preview URL: https://dac24123.devenv.pages.dev
Branch Preview URL: https://fix-task-order.devenv.pages.dev

View logs

devenv-tasks/src/lib.rs Outdated Show resolved Hide resolved
sandydoo and others added 2 commits November 23, 2024 03:54
Co-authored-by: Domen Kožar <domen@dev.si>
@sandydoo sandydoo changed the title tasks: reverse task order tasks: pickup outgoing task dependencies Nov 23, 2024
@sandydoo
Copy link
Member Author

sandydoo commented Nov 23, 2024

While this change makes setting up tasks more predictable, we'll need to make this behavior configurable to support one-off tasks. You might want to run just the specified task, or all the tasks up to the specified task.

This also opens up the question of different types of constraints, e.g. ordering vs dependency. systemd has After and Requires.

We need to brainstorm on this one.

Only doing incoming (outgoing) edges means that only before (after) works for adding a task to the graph.

Using undirected lets us traverse the entire graph, at the cost of not letting us run subgraphs of tasks. We can't tell the difference between ordering hints and requirements.

In this example, if we run devenv:enterShell in the context of launching a shell, we want to run every task reachable from it. This includes the tasks specified using after: app:afterVenv and app:afterShell.

graph TD;
    devenv:python:virtualenv-->app:afterVenv;
    devenv:python:virtualenv-->devenv:enterShell;
    devenv:enterShell-->app:afterShell;
    devenv:git-hooks:install-->devenv:enterShell;
Loading

But if we run just app:afterVenv, we may reasonable expect to only run that and devenv:python:virtualenv. Now devenv:python:virtualenv might not make sense without devenv:enterShell, but that's a spec issue.

graph TD;
    devenv:python:virtualenv-->app:afterVenv;
    devenv:python:virtualenv-->devenv:enterShell;
    devenv:enterShell-->app:afterShell;
    devenv:git-hooks:install-->devenv:enterShell;
    style app:afterVenv fill:#0095b6,stroke-width:0,color:white
    style devenv:python:virtualenv fill:#0095b6,stroke-width:0,color:white
Loading

@domenkozar
Copy link
Member

@sandydoo maybe hooks should do the former while devenv tasks run do the latter?

@domenkozar
Copy link
Member

@sandydoo shall we open an issue and merge this?

@sandydoo sandydoo marked this pull request as ready for review November 25, 2024 13:53
@sandydoo
Copy link
Member Author

@sandydoo maybe hooks should do the former while devenv tasks run do the latter?

Yeah, we need a CLI flag to modify the "traversal strategy"

@domenkozar domenkozar merged commit 6139463 into main Nov 25, 2024
262 of 277 checks passed
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.

Cannot run task after poetry install
2 participants