Skip to content

fix: the graph calculation with post_depends.#4539

Closed
boris-smidt-klarrio wants to merge 4 commits into
jdx:mainfrom
klarrio-oss:fix-post-dependency-hanging
Closed

fix: the graph calculation with post_depends.#4539
boris-smidt-klarrio wants to merge 4 commits into
jdx:mainfrom
klarrio-oss:fix-post-dependency-hanging

Conversation

@boris-smidt-klarrio

Copy link
Copy Markdown
Contributor

This bugfix fixes the graph calculation it seems that the 'post_depends' where incorrectly handled resulting in tasks where all dependending on eachother:

["task1"]
depends_post=[
    'task2',
    'task3',
    'task4',
]

["task2"]
run = "i'm task 2"

["task3"]
run = "i'm task 3"

["task4"]
run = "i'm task 4"
DEBUG Graph {
    Ty: "Directed",
    node_count: 3,
    edge_count: 6,
    edges: (2, 0), (1, 0), (1, 2), (2, 1), (0, 2), (0, 1),

@boris-smidt-klarrio

Copy link
Copy Markdown
Contributor Author

It fixes:
#4398

@boris-smidt-klarrio boris-smidt-klarrio changed the title Fix the graph calculation with post_depends. fix: the graph calculation with post_depends. Feb 26, 2025
@jdx

jdx commented Feb 26, 2025

Copy link
Copy Markdown
Owner

looks identical to #4404

@boris-smidt-klarrio

boris-smidt-klarrio commented Feb 26, 2025

Copy link
Copy Markdown
Contributor Author

Not sure why this isn't correct?
If i understand the code correct a depends_post means that it adds an extra depends to the task it references in its depends_post.

I.e. the dependency is 'inversed'.

@boris-smidt-klarrio

boris-smidt-klarrio commented Feb 26, 2025

Copy link
Copy Markdown
Contributor Author

Oke so the issue is that this will result in

/home/boris/projects/mise/target/debug/mise task deps one two three
one
two
└── one
three
└── one

So when you run two and three, it would run one even tough it isn't in the running tasks.

So what we actually want is:

/home/boris/projects/mise/target/debug/mise task deps one
one
two
└── one
three
└── one

but when you have the dependencies of two you want to get:

/home/boris/projects/mise/target/debug/mise task deps two
two

So maybe we can correct this by looking at the post deps first, if there is a post_dependency
We need to resolve it at the end to see which 'tasks' with post_dependencies need to run to add the dependency?

@boris-smidt-klarrio

boris-smidt-klarrio commented Feb 26, 2025

Copy link
Copy Markdown
Contributor Author

I suspect the post_dependency is really a feature that increases the changes by a lot that you get 'cyclical' graphs.

Tracing the code:
https://github.com/klarrio-oss/mise/blob/e12b3469e3940e70cb550c66206c754ed7bd0772/src/task/deps.rs#L22-L22
https://github.com/klarrio-oss/mise/blob/e12b3469e3940e70cb550c66206c754ed7bd0772/src/cli/run.rs#L268-L268
https://github.com/klarrio-oss/mise/blob/e12b3469e3940e70cb550c66206c754ed7bd0772/src/cli/run.rs#L226-L226

The tasks vector we get to Deps::new is actually the tasks that have to run.
So in the code a list of tasks that needs to be run is created in all_tasks_to_run which is all tasks + depends.
So then the extra depends in created by adding post_depends to this list as a result of this change.

@boris-smidt-klarrio

Copy link
Copy Markdown
Contributor Author

@boris-smidt-klarrio

Copy link
Copy Markdown
Contributor Author

I've run the deps command locally and that one is still incorrect:

mise task deps one
# one

mise task deps two
# two

mise task deps one two
# one
# two
# └── one

So i suppose the deps needs to be updated that it shows

mise task deps one
# one
# two
# └── one
# three
# └── one

@boris-smidt-klarrio

Copy link
Copy Markdown
Contributor Author

@jdx Is there any way we can move this forward, it fixes the post_depends lockup bug.

@boris-smidt-klarrio

Copy link
Copy Markdown
Contributor Author

I hope you can find some time to have another look at this to atleast have a better working post_dependency instead of a 'fully broken' version of it.

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.

2 participants