Skip to content

Fix/Add check_output_timestamps to PythonEnvironment #4113

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

Merged
merged 3 commits into from
Jun 27, 2025

Conversation

Luc-Mcgrady
Copy link
Contributor

@Luc-Mcgrady Luc-Mcgrady commented Jun 21, 2025

restat defaults to 1 (Though the docs don't make it seem that way?) meaning that .check_output_timestamps() doesn't do anything.

This, for me, fixes the python environment rebuilding every run.

@iamllama
Copy link
Contributor

I can't seem to reproduce this, n2 (from ./tools/install-n2) reports no work to do after a second ./run

@Luc-Mcgrady
Copy link
Contributor Author

Luc-Mcgrady commented Jun 21, 2025

Here is the difference this pr makes for me:

Peek.2025-06-21.23-53.mp4

Is there something to do with ./tools/install-n2 that I'm meant to do to avoid this behaviour?

On main this is the output of ./ninja -d explain -v

    Finished `release` profile [optimized] target(s) in 0.15s
ninja explain: output /home/luc/Programming/forks/anki/out/pyenv/bin/black older than most recent input /home/luc/Programming/forks/anki/out/rust/release/runner (1750527715838813748 vs 1750546233608145463)
ninja explain: /home/luc/Programming/forks/anki/out/pyenv/bin/protoc-gen-mypy is dirty
ninja explain: /home/luc/Programming/forks/anki/out/pylib/anki/ankidroid_pb2.py is dirt
ninja explain: /home/luc/Programming/forks/anki/out/pylib/anki/ankidroid_pb2.pyi is dirty
...

@iamllama
Copy link
Contributor

Is there something to do with ./tools/install-n2 that I'm meant to do to avoid this behaviour?

n2 is intended to be used instead of ninja (which doesn't support the hide_success rule var that ninja_gen emits, for example), and is installed via that script

@Luc-Mcgrady
Copy link
Contributor Author

using n2 fixes this for me but It's still worth fixing for ninja users right?

@iamllama
Copy link
Contributor

Dug into this a bit (discussed over discord), and from looking at ninja's source it seems to check for the restat rule var's presence (not its value) as the docs state
https://github.com/ninja-build/ninja/blob/656412538b6fc102b809a61e0efce422e5a20534/src/graph.cc#L514
https://github.com/ninja-build/ninja/blob/656412538b6fc102b809a61e0efce422e5a20534/src/build.cc#L912

Which means that every statement ends up getting restat'ed, which fixes the issue but not in an expected way 🤔

@Luc-Mcgrady Luc-Mcgrady reopened this Jun 22, 2025
@Luc-Mcgrady Luc-Mcgrady changed the title Fix/Always explicitly set restat Fix/Add check_output_timestamps to PythonEnvironment Jun 22, 2025
@dae
Copy link
Member

dae commented Jun 27, 2025

The build scripts try to support both n2 and ninja. @iamllama I'm not sure I'm following your comment - I thought you meant we were doing restat = 0 (which wouldn't work), but the code appears to be only conditionally writing it:

    if action.check_output_timestamps() {
        stmt.rule_variables.push(("restat".into(), "1".into()));
    }

Could you clarify?

@dae
Copy link
Member

dae commented Jun 27, 2025

Thanks Luc!

@dae dae merged commit e505ca0 into ankitects:main Jun 27, 2025
1 check passed
@iamllama
Copy link
Contributor

iamllama commented Jun 27, 2025

I thought you meant we were doing restat = 0 (which wouldn't work), but the code appears to be only conditionally writing it:

@dae I was referring to the pr's original impl ace2e5e which would have always defined restat

@dae
Copy link
Member

dae commented Jun 27, 2025

Ah, gotcha. I was familiar with this issue, as I made the same mistake with assuming 0 = off with n2 in the past. 😅

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.

3 participants