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

Dyndep patches #2215

Closed
wants to merge 2 commits into from
Closed

Dyndep patches #2215

wants to merge 2 commits into from

Conversation

dfgtyx
Copy link

@dfgtyx dfgtyx commented Nov 12, 2022

This request contains two patches. The first adjusts "ninja -t cleandead" to account for dynamic dependencies. By neglecting dynamic dependencies, Ninja spuriously flags artifacts as "dead" thereby increasing the cost of rebuilds down the road. The second patch improves the "ninja clean" performance for projects with dynamic dependencies. With this patch, on one of my projects I have observed runtimes for "ninja clean" drop from 5 minutes to 25 seconds.

Fix for a bug causing artifacts to be spuriously flagged as "dead" and
erased.
Added code to check "pending" so as to avoid loading data that has already
is already loaded.
@dfgtyx
Copy link
Author

dfgtyx commented Nov 17, 2022

RPM failed to install something from EPEL. I think this has nothing to do with the PR. Is the automation malfunctioning? If not, then I may need some help understand the error.

@dfgtyx
Copy link
Author

dfgtyx commented Jan 27, 2023

Bump.

@jhasse jhasse added this to the 1.12.0 milestone Feb 7, 2023
@jhasse
Copy link
Collaborator

jhasse commented Feb 7, 2023

@bradking Can you have a look?

@bradking
Copy link
Contributor

bradking commented Feb 7, 2023

The changes look correct to me. For reference, dyndep support was added by #1521, including support for clean in commit a3cbb4d. Then #1432 was merged later to add cleandead, but was not updated to account for the changes made to clean by #1521. This PR's change to Cleaner::CleanDead makes that update correctly.

@dfgtyx please revise your commit messages to describe this history:

cleandead: remove outputs specified by dyndep files

Load this information before cleaning, as the normal `clean` operation
does.  This was accidentally missed when commit 714621db (Adding a way
to clean dead build artifacts ..., 2018-04-27, v1.10.0~1^2~9^2~1) was
rebased after the normal `clean` operation was updated by commit a3cbb4d
(clean: remove outputs specified by dyndep files, 2019-02-12,
v1.10.0~1^2~53^2~4).

This fixes a bug causing artifacts to be spuriously flagged as "dead"
and erased.

and

clean: Improve performance in presence of dynamic dependencies

Add code to check "pending" so as to avoid loading dyndep files that
have already been loaded.  This was missed by commit a3cbb4d (clean:
remove outputs specified by dyndep files, 2019-02-12, v1.10.0~1^2~53^2~4).

Also please look at updating src/clean_test.cc with a CleanDeadDyndep case similar to the existing CleanDyndep case.

@jhasse
Copy link
Collaborator

jhasse commented Aug 23, 2023

Does this fix #1952, too?

@jhasse
Copy link
Collaborator

jhasse commented Nov 22, 2023

@dfgtyx Can you adjust the commit message and force push?

@jhasse jhasse modified the milestones: 1.12.0, 1.13.0 Feb 29, 2024
@dcbaker
Copy link
Contributor

dcbaker commented Mar 29, 2024

Is there anything I can do to help move this along? I just ran into this while working on dyndep support in Meson, wrote the same patches, and then found this before sending them.

@jhasse
Copy link
Collaborator

jhasse commented Apr 2, 2024

@dcbaker Can you creater a second PR? We could merge that one for 1.12.0.

@dcbaker
Copy link
Contributor

dcbaker commented Apr 2, 2024

@jhasse, done as 2406

@jhasse jhasse modified the milestones: 1.13.0, 1.12.0 Apr 2, 2024
@jhasse
Copy link
Collaborator

jhasse commented Apr 2, 2024

closing in favor of #2406

@jhasse jhasse closed this Apr 2, 2024
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.

None yet

4 participants