zstd
zstd copied to clipboard
facebook
Don't attempt playTests.sh cmake test if running on Windows.
Disable this test on Windows because it runs a bash script. I'm not sure of a way to easily determine if the shell is bash, so I have just disabled it on Windows.
It seems there is already a property flag to decide running playTests.sh or not.
Maybe you could re-use this mechanism ?
Also, I'm not sure if "Windows" is a good enough discriminant for such a decision.
For example, running a bash shell on Windows works fine when using environments like mingw and cygwin, so they should not be impacted by this decision.
Instead, what you may mean is "Windows + Visual Studio + MS Command Prompt".
I'm concerned that WIN32 doesn't capture this difference.
Finally, maybe we need a CI test to ensure this property is running as intended and continues to run as intended in the future, when future contributors change some part of the code base.
It seems there is already a property flag to decide running playTests.sh or not.
Are you referring to ZSTD_PLAYTESTS_FLAGS? That appears to be just flags to pass into the shell script. The shell script will get executed even if no ZSTD_PLAYTESTS_FLAGS are passed.
As it currently sits this cmake tests would fail to run on Windows, using cmd or PowerShell or Windows Terminal.
I don't think that is it... I'm not sure how I would enable/disable that. I could do a check for uname?
find_program(UNAME uname)
if (UNAME)
// Must be shell terminal
else()
// Must not be
endif()
I could do a check for
uname?
uname is likely safer.
I would expect this command to offer a clearer distinction between Microsoft environments, like cmd and powershell, and posix ones like msys2 and cygwin.
Hi @nmoinvaz!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
There is another problem, that playTest.sh is triggered when the project is added with EXCLUDE_FROM_ALL.
add_subdirectory(${ZSTD_SOURCE_DIR}/build/cmake ${ZSTD_BINARY_DIR} EXCLUDE_FROM_ALL)
The problem is that datagen and apps are not built with EXCLUDE_FROM_ALL.
Also is it weird that add_test still happens for playTest even if ZSTD_BUILD_PROGRAMS is OFF? Seems like the list of tests could be built as a list and playTest added conditionally. Instead of creating the test and then disabling it..
I fixed the logic around the uname detection, but don't have a solution for detecting if datagen will be built.
Seems like the list of tests could be built as a list and playTest added conditionally. Instead of creating the test and then disabling it..
Looks like using DISABLED might be better because the output looks like:
Start 300: playTests
300/301 Test #300: playTests ........................***Not Run (Disabled) 0.00 sec
I fixed the logic around the uname detection, but don't have a solution for detecting if datagen will be built.
Does it matter if datagen is built ?
I presume the only issue is about running the shell script.
Does it matter if datagen is built?
Yes, it does matter because playTests.sh depends on datagen and runs it. If include zstd using add_subdirectory(zstd EXCLUDE_FROM_ALL) then datagen will not be built but playTests.sh will be run in CTest.
I presume the only issue is about running the shell script.
Correct. Now this PR makes it so that playTest.sh only runs in unix environment. This is different from the datagen issue. It looks like the problem with EXCLUDE_FROM_ALL and CTest has been run into before. I have documented your use case here.
