-
Notifications
You must be signed in to change notification settings - Fork 90
Re-trying the PR #881: Migrating native sim from GCC to Clang. #889
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…CI build from scratch.
anpaz
left a comment
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.
lgtm.
|
Please review. |
| set(TEST_DEPS1 $ENV{NATIVE_SIMULATOR}) | ||
| else() | ||
| set(TEST_DEPS1 "${PROJECT_SOURCE_DIR}/../../Simulation/native/build/$ENV{BUILD_CONFIGURATION}") | ||
| set(TEST_DEPS1 "${PROJECT_SOURCE_DIR}/../../Simulation/native/build/drop") |
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.
@kuzminrobin Why did this path change? Seems like the CI should have had issues before or after.
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.
When the native simulator was built with MSVC (on Win), the compiler was putting the binary to
Simulation/native/build/Release (whereas the GCC on other platforms was putting the binary to
Simulation/native/build). Then the scripts were copying the binary to
Simulation/native/build/drop (on all platforms), but the dependency in the tests was different for win and other platforms.
After the native simulator has been migrated to Clang, on all the platforms the binary started being put
Simulation/native/build and copied to
Simulation/native/build/drop. This change has unified the dependency.
|
Any more feedback please? |
| { | ||
| Write-Host "On Linux build native simulator using gcc (needed for OpenMP)" | ||
| cmake -D BUILD_SHARED_LIBS:BOOL="1" -D CMAKE_C_COMPILER=gcc -D CMAKE_CXX_COMPILER=g++ -D CMAKE_BUILD_TYPE="$Env:BUILD_CONFIGURATION" .. | ||
| cmake -D BUILD_SHARED_LIBS:BOOL="1" -D CMAKE_C_COMPILER=clang-11 -D CMAKE_CXX_COMPILER=clang++-11 ` |
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 like sanitizer flags are being added even for release builds, which we don't want. The sanitizers should only be enabled for debug builds or only in the sanitizer pipeline, we don't want to ship binaries on linux and mac that have sanitizers enabled.
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.
Not exactly, The sanitizer flags are only added for DEBUG, see below:
CMAKE_C_FLAGS_DEBUG="$SANITIZE_FLAGS"
CMAKE_CXX_FLAGS_DEBUG="$SANITIZE_FLAGS"
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.
Ah, I misread, you are correct!
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.
Can you please uncheck the "Change requested" flag?
A set of 3 PRs: Q#RT, QDK, IQ# repos (see also quantum-docs PR).
A set of the latter 2 are expected to require a branch-specific e2e build or a merge of the former one first.