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

llbuild: Prevent reported build tasks count from decreasing #7329

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rauhul
Copy link
Contributor

@rauhul rauhul commented Feb 10, 2024

Currently SwiftPM will reduce the total number of jobs in the CLI output if llbuild determines a task does not need to be run. This results in confusing output to the user where as the build progresses the number of completed tasks strictly increases but the total can fluctuate. This commit updates the task counting logic to consider "up to date" build operations as complete instead of reducing the total count leading to clearer progress output.

Currently SwiftPM will reduce the total number of jobs in the CLI output
if llbuild determines a task does not need to be run. This results in
confusing output to the user where as the build progresses the number of
completed tasks strictly increases but the total can fluctuate. This
commit updates the task counting logic to consider "up to date" build
operations as complete instead of reducing the total count leading to
clearer progress output.
@rauhul
Copy link
Contributor Author

rauhul commented Feb 10, 2024

@swift-ci test

MaxDesiatov
MaxDesiatov previously approved these changes Feb 11, 2024
Copy link
Member

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks!

@MaxDesiatov MaxDesiatov changed the title Prevent build tasks from decreasing llbuild: Prevent reported build tasks count from decreasing Feb 11, 2024
@MaxDesiatov MaxDesiatov added bug build system Changes to interactions with build systems needs tests This change needs test coverage labels Feb 11, 2024
Copy link
Member

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

On a closer look, can we come up with a test case for this?

@MaxDesiatov MaxDesiatov self-requested a review February 11, 2024 10:53
@MaxDesiatov
Copy link
Member

@swift-ci test linux

@MaxDesiatov MaxDesiatov dismissed their stale review February 11, 2024 10:54

The change doesn't seem to have corresponding test cases that verify the desired behavior.

@compnerd
Copy link
Collaborator

Tree pruning is pretty common in build systems (e.g. make, ninja). I think that the more confusing (and frustrating) aspect is the increasing target counts.

@rauhul
Copy link
Contributor Author

rauhul commented Feb 12, 2024

Tree pruning is pretty common in build systems (e.g. make, ninja). I think that the more confusing (and frustrating) aspect is the increasing target counts.

Thats fine, but the fact that the total count first increases then decreases is pretty poor/confusing user experience. We could add a "skipped" count (which I actually plan on doing in a future PR) but that would not be "ninja" style output

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug build system Changes to interactions with build systems needs tests This change needs test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants