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 reverse dep. tracking for alwaysRerun rules #2298

Merged
merged 1 commit into from
Oct 24, 2021
Merged

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Oct 24, 2021

When I ported reverse dependencies from Shake[1] I missed an important
detail. While Shake models alwaysRerun as a dependency on an actual rule
(AlwaysRerun), hls-graph models it by setting actionDeps to
Nothing. This is important difference - it means dependencies are not computed for
these rules, and therefore reverse dependency tracking doesn't do
anything, which breaks correctness of dirty rebuilds.

This commit adds dependency tracking for alwaysRerun rules, and fixes
reverse dependency tracking. The alternative would be to follow the
Shake approach but I'm not sure what other implications this might have.

[1] - ndmitchell/shake#802

When I ported reverse dependencies from Shake[1] I missed an important
detail. While Shake models alwaysRerun as a dependency on an actual rule
(AlwaysRerun), hls-graph models alwaysRerun by setting actionDeps to
Nothing. This is important because dependencies are not computed for
these rules, and therefore reverse dependency tracking doesn't do
anything, which breaks correctness of dirty rebuilds

This commit adds dependency tracking for alwaysRerun rules, and fixes
reverse dependency tracking. The alternative would be following the
Shake approach but I'm not sure what other implications this might have.

[1] - ndmitchell/shake#802
@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Oct 24, 2021
@pepeiborra pepeiborra merged commit 9233be8 into master Oct 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants