Test the build options #790
Conversation
|
The precommit fail related to #762. |
|
@bzsolt, please rebase to the current master |
bzsolt
changed the title
| @@ -113,9 +113,9 @@ project (Jerry C ASM) | |||
| set(MCU_SCRIPT_GENERATED_HEADER ${CMAKE_BINARY_DIR}/generated.h) | |||
|
|
|||
| # Should we use external libc? | |||
| if(DEFINED USE_COMPILER_DEFAULT_LIBC AND USE_COMPILER_DEFAULT_LIBC STREQUAL "YES") | |||
| if(DEFINED COMPILER_DEFAULT_LIBC AND COMPILER_DEFAULT_LIBC STREQUAL "ON") | |||
There was a problem hiding this comment.
Why did you rename this? Are you sure you changed all reference of USE_COMPILER_DEFAULT_LIBC?
There was a problem hiding this comment.
Yes, I've changed all occurences of USE_COMPILER_DEFAULT_LIBC.
The YES/NO -> ON/OFF modification just simplify the testing-mechanism, because only this option use YES/NO, the others use ON/OFF.
In my opinion, the ON/OFF is more obvious without the USE_ prefix.
There was a problem hiding this comment.
OK, thanks for explanation.
|
LGTM |
|
Sorry wrong PR :) |
| @@ -332,6 +349,8 @@ precommit: prerequisites | |||
| $(Q)+$(MAKE) --no-print-directory clean | |||
| $(Q) echo "Running checks..." | |||
| $(Q)+$(MAKE) --no-print-directory check-signed-off check-vera check-cppcheck | |||
| $(Q) echo "...testing build-options..." | |||
| $(Q)+$(MAKE) --no-print-directory test-buildoptions | |||
There was a problem hiding this comment.
I'm concerned about performance here. I quite often just run make (== make precommit) to see that everything is all right (checks, builds, tests). However, with this addition, this will perform all the builds with various options too. And this will call out to a shell script, which sequentially walks through targets and options, and then calls back to make again, thus loosing all parallelization potential. (Not to mention that ninja builds are way faster than non-ninja builds, but even if I run make precommit NINJA=1, because of the make->shell->make calls, this setting will be lost and loose ninja as well.) Is there no way to get everything done within the Makefile?
There was a problem hiding this comment.
You're right, I will check the way to move this logic to the Makefile.
There was a problem hiding this comment.
It should be discussed and decided whether we want this to be included in the precommit target or have a separate target for the "heavy" checks (as was the case in a previous version, if I recall correctly). What's the impact of this addition on the precommit time? (Ofc, it can also be a valid answer that we don't care and want this tested, whatever it takes.)
There was a problem hiding this comment.
With this version the runtime is more than twice before.
But now when the option-testing running in parallel and the build-option step included into the precommit process, the runtime only increased to ~5:55 minutes from ~3:25 on my local machine. So basically option-testing takes ~2:30 minutes.
What do you think, is this slow-down acceptable or not?
|
I've updated the PR, @akiss77 could you check it please? |
| test-buildoptions: | ||
| $(Q) for target in $(JERRY_BUILD_OPTIONS_TEST_TARGETS); do \ | ||
| if [[ $$target =~ "mcu" ]]; then \ | ||
| current_options=$$BUILD_OPTIONS_TEST_MCU; \ |
There was a problem hiding this comment.
We are in the Makefile, so we can use $(BUILD_OPTIONS_TEST_MCU), we don't have to rely on the make variable being exported to a shell variable.
| @@ -196,7 +216,9 @@ define WRITE_TOOLCHAIN_CONFIG | |||
| endef | |||
|
|
|||
| .PHONY: $(BUILD_DIR)/$(NATIVE_SYSTEM)/toolchain.config | |||
| $(BUILD_DIR)/$(NATIVE_SYSTEM)/toolchain.config: | |||
| .PHONY: $(BUILD_DIR)/$(NATIVE_SYSTEM)/unittests/toolchain.config | |||
There was a problem hiding this comment.
The cause of this (and related) modifications is that the parallel native builds can interfere with the unittests build.
|
LGTM (please rebase) |
|
LGTM |
|
LGTM |
Add new build target: test-buildoptions Now every build option is tested on the proper targets. These are the native and the MCU targets in release mode and unittests. Therefore the USE_COMPILER_DEFAULT_LIBC build option is refactored. JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély [email protected]
Now the precommit checks only the {release,debug} native targets and unittests
with all combinations. Because testing on every target needs a lots of efforts,
added a new target called "precommit_heavy".
This target checks all targets with every combinations.
JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély [email protected]