bpo-42207: Make llvm profile data location reference absolute #23037
Conversation
configure.ac
Outdated
| @@ -1460,8 +1460,8 @@ case $CC in | |||
| *clang*) | |||
| # Any changes made here should be reflected in the GCC+Darwin case below | |||
| PGO_PROF_GEN_FLAG="-fprofile-instr-generate" | |||
| PGO_PROF_USE_FLAG="-fprofile-instr-use=code.profclangd" | |||
| LLVM_PROF_MERGER="${LLVM_PROFDATA} merge -output=code.profclangd *.profclangr" | |||
| PGO_PROF_USE_FLAG="-fprofile-instr-use=$PWD/code.profclangd" | |||
There was a problem hiding this comment.
I would prefer to use $(abs_builddir): I am not sure that $PWD is portable on any shell.
Would you mind to add a comment referecing bpo-42207 to explain than an absolute path allows to run the test suite outside the build directory?
Is the problem that pegen uses "NODIST" variables like CFLAGS_NODIST and LDFLAGS_NODIST?
There was a problem hiding this comment.
I would even prefer to deactivate this test on pgo builds or if detects that is running under a profile generation run given that it adds nothing useful and it takes a considerable time to execute.
There was a problem hiding this comment.
test_peg_generator is not part of the default profile task (./python -m test --pgo: test_peg_generator is not part of Lib/test/libregrtest/pgo.py). The problem is once Python it built, running test_peg_generator outside the build directory fails, according to https://bugs.python.org/issue42207
There was a problem hiding this comment.
I know, what I am proposing is to skip it if it ever runs under a profile generation step
There was a problem hiding this comment.
Two possible approaches: https://sergesanspaille.fedorapeople.org/0001-Disable-peg-generator-test-under-PGO.patch and https://sergesanspaille.fedorapeople.org/0001-Fix-peg-parser-in-presence-of-profile-data.patch , which one do you prefer?
There was a problem hiding this comment.
I assume that the first approach will be without the "Monkey patch CFLAGS" part, no?
In that case, I think I prefer the first one, as this test is particularly heavy with absolutely no gain on profile-based builds, it will benefit folks running the full test suite for PGO
There was a problem hiding this comment.
@pablogsal your assumption was correct. I picked that patch and updated the PR accordingly.
How so? The master branch has exactly the same test and the same way of generating profile data. |
serge-sans-paille
force-pushed
the
3.9
branch
from
80a1b9e to
3d97304
Compare
oh, for some reasons these files were deleted on my local setup. Retargeting that PR to the master branch then |
serge-sans-paille
requested review from
1st1,
abalkin,
asvetlov,
berkerpeksag,
brettcannon,
encukou,
ericsnowcurrently,
ericvsmith,
ethanfurman,
gpshead,
gvanrossum,
ilevkivskyi,
markshannon,
methane,
ncoghlan,
pganssle,
rhettinger,
terryjreedy,
tiran,
vsajip,
warsaw and
a team
as code owners
serge-sans-paille
force-pushed
the
3.9
branch
from
3d97304 to
e0220cc
Compare
Otherwise, when running the testsuite, test_peg_generator tries to compile C code using the optimized flags and fails because it cannot find the profile data. https://bugs.python.org/issue42207
serge-sans-paille
force-pushed
the
3.9
branch
from
e0220cc to
f504307
Compare
|
@serge-sans-paille can you open a new PR? When you rebase it messes up GitHub and then a ton of people get added as reviewers. |
Otherwise, when running the testsuite, test_peg_generator tries to compile C
code using the optimized flags and fails because it cannot find the profile
data.
https://bugs.python.org/issue42207