Skip to content

Comments

GH-145110: fix arguments Clean and CleanAll for argument -t of batch.bat in case of PGO builds on Windows#145111

Merged
zooba merged 1 commit intopython:mainfrom
chris-eibl:fix_pgo_clean
Feb 23, 2026
Merged

GH-145110: fix arguments Clean and CleanAll for argument -t of batch.bat in case of PGO builds on Windows#145111
zooba merged 1 commit intopython:mainfrom
chris-eibl:fix_pgo_clean

Conversation

@chris-eibl
Copy link
Member

@chris-eibl chris-eibl commented Feb 22, 2026

In case of Clean or CleanAll, the

  • the pgo_job must not be invoked
  • and the target must not be switched to Build

@chris-eibl

This comment was marked as outdated.

@bedevere-bot

This comment was marked as outdated.

@chris-eibl

This comment was marked as outdated.

@bedevere-bot

This comment was marked as outdated.

call "%dir%\..\python.bat" %pgo_job%
@echo off
call :Kill
set target=Build
Copy link
Member Author

Choose a reason for hiding this comment

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

I do not really know why the target is switched here. For Clean and CleanAll this is definitely wrong. This only leaves Rebuild which is still converted to Build here like before.

I'd happily remove it so that Rebuild does what is asked for, and a quick test shows that it works.
Git blame reveals it was introduced in this commit linked to https://bugs.python.org/issue28573, which is dealing about PATH getting to long and

[...] also fixes two other issues with my build script - the PGOOPTS need to come before CERTOPTS, as they are parsed by different scripts, and the debug build command fell off somewhere which resulted in the debug builds not being rebuilt for 3.6.0b4 - they are still the 3.6.0b3 debug build.

Yet, I am unsure and might be overlooking something, so I haven't touched it (yet).

Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm misremembering how this works (I almost never rely on this batch file anymore - real releases don't use it at all), Rebuild here would cause recompiling post-profile, which isn't necessary. All the PGO happens during link, so we should save about half the build time by not rebuilding things that don't need rebuilding.

We would want Rebuild during PGInstrument, since that's the first stage. But from then, it's already re-built, and so subsequent stages shouldn't Rebuild.

Copy link
Member Author

@chris-eibl chris-eibl Feb 23, 2026

Choose a reason for hiding this comment

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

Ok, then let's keep it. Then I think this is fine to merge? I'll take care about the long path problem of the AMD64 Windows PGO NoGIL Tailcall PR builder with @itamaro in a discord discussion. I am close to 100% sure the regular builders will be green (like AMD64 Windows PGO NoGIL PR is for this PR as well).

@chris-eibl

This comment was marked as outdated.

@chris-eibl

This comment was marked as outdated.

@bedevere-bot

This comment was marked as outdated.

@chris-eibl

This comment was marked as off-topic.

@itamaro

This comment was marked as off-topic.

@itamaro

This comment was marked as off-topic.

@itamaro

This comment was marked as off-topic.

@chris-eibl

This comment was marked as off-topic.

@chris-eibl

This comment was marked as off-topic.

@chris-eibl

This comment was marked as off-topic.

@chris-eibl chris-eibl changed the title GH-145110: fix targets "Clean" and "CleanAll" in case of Windows PGO builds GH-145110: fix arguments Clean and CleanAll for argument -t of batch.bat in case of PGO builds on Windows Feb 23, 2026
@zooba zooba merged commit 0e4b1ba into python:main Feb 23, 2026
50 of 51 checks passed
@chris-eibl chris-eibl deleted the fix_pgo_clean branch February 23, 2026 23:03
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.

4 participants