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

graph: always use the on-disk mtime of generator rules #1527

Conversation

mathstuf
Copy link
Contributor

@mathstuf mathstuf commented Feb 4, 2019

Generator rules are likely to have been run outside of ninja, so the
build log should not be trusted for detecting whether or not to rerun
the rule.


Observed with CMake projects which exhibit behavior that ninja reruns cmake right after running cmake manually. Maybe it is better to just not add generator rules to the build log at all? I don't know of the implications of that.

See https://gitlab.kitware.com/cmake/cmake/issues/15830 for some context.

@fkaelberer
Copy link

fkaelberer commented Feb 11, 2019

So this should fix #1376, I guess.

@mathstuf
Copy link
Contributor Author

Yes, it should. I suspect there are likely other duplicates of the issue around.

@Siron777
Copy link

I have tested the patch and I do not observe anymore the issue #1376.

src/graph.cc Show resolved Hide resolved
Generator rules are likely to have been run outside of ninja, so the
build log should not be trusted for detecting whether or not to rerun
the rule.
@mathstuf mathstuf force-pushed the always-check-generator-disk-mtime branch from ab5712a to 29a565f Compare March 1, 2019 13:23
@rubenvb
Copy link

rubenvb commented Mar 5, 2019

This seems to fix issue #1376 for me.

@jhuels
Copy link
Contributor

jhuels commented Apr 5, 2019

This fixes the issue for generator rule for me, but I don't think CMake provides a way to mark custom commands as generator. I'm having an identical issue with a code generator where the disc time of the output is newer because it was run outside of ninja, but ninja_log is stale.

Do you know of a way around this, or should I request a feature for CMake to set custom commands as generator?

@mathstuf
Copy link
Contributor Author

mathstuf commented Apr 5, 2019

Please open an issue over there (rather than muddying the discussion here), but include your example custom command. Ninja's rules on the generator rule variable doesn't really make sense with custom commands.

@jhasse
Copy link
Collaborator

jhasse commented Apr 5, 2019

Do you know of a way around this

Update the ninja_log in your custom command.

@mathstuf
Copy link
Contributor Author

mathstuf commented Apr 8, 2019

Update the ninja_log in your custom command.

I don't know that this is going to be easy. Ninja itself uses the tmpfile-and-rename style, so any command doing this would need to coordinate with ninja on what's going on. This just sounds 100% flaky to me.

@jhasse
Copy link
Collaborator

jhasse commented Apr 8, 2019

Ninja itself uses the tmpfile-and-rename style, so any command doing this would need to coordinate with ninja on what's going on.

But this shouldn't matter because we're talking about the case where the generator is run outside of ninja, right?

@mathstuf
Copy link
Contributor Author

mathstuf commented Apr 8, 2019

How is the tool supposed to know if it is inside or outside ninja? Plus, the question asked was for a way to claim generator = 1 in order to access the behavior introduced here. Maybe some other flag should be added that generator = 1 should imply?

@jhasse
Copy link
Collaborator

jhasse commented Apr 8, 2019

How is the tool supposed to know if it is inside or outside ninja?

Pass itself a flag in its build rule for example.

@mathstuf
Copy link
Contributor Author

mathstuf commented May 3, 2019

Maybe if the deplog format were documented and guaranteed to be stable, that route could be taken. Until then, is this a sufficient solution to the problem being seen?

@jhasse
Copy link
Collaborator

jhasse commented May 10, 2019

I would rather add a tool to Ninja (e.g. ninja -t updatecache?) or add the functionality to an existing one. What about ninja -t recompact? Or the new cleandead tool? Running the latter on reconfigures is a good idea anyway, as it might catch bugs by deleting files hanging around from previous builds.

@mathstuf
Copy link
Contributor Author

Hmm. That could work. A pipeline like ninja -t cleandead && ninja -t recompact && ninja -t restat maybe?

@jhasse
Copy link
Collaborator

jhasse commented May 13, 2019

Exactly! restat also sounds like a good name.

@mathstuf
Copy link
Contributor Author

Strawman MR filed for CMake here: https://gitlab.kitware.com/cmake/cmake/merge_requests/3316

@jhasse
Copy link
Collaborator

jhasse commented Nov 18, 2019

Closing in favor of #1685.

@jhasse jhasse closed this Nov 18, 2019
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

6 participants