-
Notifications
You must be signed in to change notification settings - Fork 692
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
[lit] use lit.py directly to allow send parameter to lit from hcttest #5438
Conversation
utils/hct/hcttest.cmd
Outdated
rem check all includes clang, dxilconv, and exec | ||
cmake --build %HLSL_BLD_DIR% --config %BUILD_CONFIG% --target check-all | ||
rem check all includes clang, dxilconv | ||
py %HLSL_SRC_DIR%/utils/lit/lit.py -sv --no-progress-bar --param build_mode=%BUILD_CONFIG% --param dxilconv_site_config=%HLSL_BLD_DIR%/projects/dxilconv/test/taef/lit.site.cfg --param clang_site_config=%HLSL_BLD_DIR%/tools/clang/test/lit.site.cfg --param skip_taef_exec=True --param no_priority=True --param llvm_site_config=%HLSL_BLD_DIR%/test/lit.site.cfg --param llvm_unit_site_config=G:/repos/DXC/hlsl.bin/test/Unit/lit.site.cfg %HLSL_SRC_DIR%/projects/dxilconv/test/taef %HLSL_BLD_DIR%/tools/clang/test %HLSL_BLD_DIR%/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the discussion yesterday, this shouldn't be the default behavior.
By default, we should run through CMake so that the tests being run are guaranteed to be up-to-date. We should only call lit directly for options that we want to support overriding via the hcttest interface, or on an opt-in basis when the user is consciously deciding to bypass updating the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So only exec test use lit.py and set no_priority=True as default for other tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok by me, assuming you resolve @llvm-beanz issues.
This is for support adapter option for exec test when using lit. Also no_priority is enabled for clang test. CMake behavior will be the same except not support adapter option.
Use cmake for default case. Remove useless lit param for exec.
2e9e09d
to
3a0fb95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'd just like the cmd change mentioned
if "%TEST_CMD%"=="1" ( | ||
py %BIN_DIR%\llvm-lit.py %HLSL_SRC_DIR%/tools/clang/test/DXC -v | ||
set RES_CMD=!ERRORLEVEL! | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another change in behavior that isn't mentioned in the description. I think it's worth mentioning therre
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
This is for support adapter option for exec test when using lit. no_priority is enabled for clang test too.
CMake behavior will be the same except not support adapter option.
Also hcttest cmd will only run cmd test for lit.