-
Notifications
You must be signed in to change notification settings - Fork 444
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
Deprecate unified build in favor of unity build. #3491
Changes from 8 commits
3c88259
f63dd67
a716473
47da77d
4d947a8
44e8b98
94cf84f
d5734d7
d7a2c55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,8 +12,8 @@ | |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
# This is the CMake version supplied by Ubuntu 18.04. | ||
cmake_minimum_required (VERSION 3.10.2 FATAL_ERROR) | ||
# This is the CMake version supplied by Ubuntu 20.04. | ||
cmake_minimum_required (VERSION 3.16.3 FATAL_ERROR) | ||
|
||
find_program(CCACHE_PROGRAM ccache) | ||
if(CCACHE_PROGRAM) | ||
|
@@ -47,6 +47,8 @@ OPTION (ENABLE_SANITIZERS "Enable sanitizers" OFF) | |
OPTION (BUILD_STATIC_RELEASE "Build a statically linked release binary" OFF) | ||
OPTION (BUILD_AUTO_VAR_INIT_PATTERN "Initialize variables with pattern during build" OFF) | ||
OPTION (ENABLE_IWYU "Enable checking includes with IWYU" OFF) | ||
# Support a legacy option. TODO: Remove? | ||
OPTION (ENABLE_UNIFIED_COMPILATION "Enable CMAKE_UNITY_BUILD" OFF) | ||
|
||
set (P4C_DRIVER_NAME "p4c" CACHE STRING "Customize the name of the driver script") | ||
|
||
|
@@ -83,13 +85,26 @@ else() | |
set (P4C_VERSION "${P4C_SEM_VERSION_STRING} (SHA: ${P4C_GIT_SHA} BUILD: ${CMAKE_BUILD_TYPE})") | ||
endif() | ||
include(P4CUtils) | ||
include(UnifiedBuild) | ||
|
||
# # search in /usr/local first | ||
# set (CMAKE_FIND_ROOT_PATH "/usr/local/bin" "${CMAKE_FIND_ROOT_PATH}") | ||
set (P4C_CXX_FLAGS "") | ||
set (P4C_LIB_DEPS) | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if (ENABLE_UNIFIED_COMPILATION) |
||
# Support the legacy unified build option. | ||
if(ENABLE_UNIFIED_COMPILATION) | ||
message( | ||
WARNING | ||
"Using deprecated option \"ENABLE_UNIFIED_COMPILATION\". Please use \"CMAKE_UNITY_BUILD\" instead." | ||
) | ||
set(CMAKE_UNITY_BUILD ON) | ||
endif() | ||
|
||
# If unity builds are enabled, choose an aggressive batch size. | ||
if (CMAKE_UNITY_BUILD) | ||
set(CMAKE_UNITY_BUILD_BATCH_SIZE 16 CACHE UNINITIALIZED "Set the unity build batch size.") | ||
endif () | ||
# set the required options for a static release build | ||
if (BUILD_STATIC_RELEASE) | ||
message(STATUS "Building static release binaries") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -235,7 +235,7 @@ inja::json::array_t STF::getClone(const std::map<cstring, const TestObject *> &c | |
return cloneJson; | ||
} | ||
|
||
static std::string getTestCase() { | ||
std::string STF::getTestCaseTemplate() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how are these changes related to unified builds? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When multiple of the test |
||
static std::string TEST_CASE( | ||
R"""(# p4testgen seed: {{ default(seed, "none") }} | ||
# Date generated: {{timestamp}} | ||
|
@@ -328,7 +328,7 @@ void STF::outputTest(const TestSpec *testSpec, cstring selectedBranches, size_t | |
auto incrementedTestName = testName + "_" + std::to_string(testIdx); | ||
|
||
stfFile = std::ofstream(incrementedTestName + ".stf"); | ||
std::string testCase = getTestCase(); | ||
std::string testCase = getTestCaseTemplate(); | ||
emitTestcase(testSpec, selectedBranches, testIdx, testCase, currentCoverage); | ||
} | ||
|
||
|
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.
Will it run? dont you need sudo -E?
i am suprised why you need any changes at all in this file
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.
Some hours ago when I was looking why validate fails it was due to some leftover of removed build_unified.
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.
Try to avoid any changes here and see what CI says
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.
Yes it should run without sudo.
sudo
is only required for eBPF kernel tests. These changes need to be preserved to avoid build failures of Gauntlet affecting PRs. This is because Gauntlet is a downstream dependency.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.
But this is still broken. Validate now skips Validate or Build failed step (but build went fine, test image was created so uhm, strange…. what is worse, it is green, so easy to overlook any issues - I changed it in my previous PR but I am not sure why you reverted those changes).
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.
Yes, that is intentional. Failed builds should not show up in PRs since they can only be fixed in the downstream repository.
I will fix the odd skipping.