Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ci: remove clang workflow, use main twister workflow to build using llvm #82354

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+24 −169
Open

ci: remove clang workflow, use main twister workflow to build using llvm #82354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d4edc1
twister: add an option to force a toolchain variant for a platform
nashif Nov 29, 2024
60cb67c
ci: twister: build native_sim with llvm
nashif Nov 29, 2024
e3a87ba
ci: clang: remove clang workflow
nashif Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 0 additions & 165 deletions .github/workflows/clang.yaml
View file
Open in desktop

This file was deleted.

15 changes: 12 additions & 3 deletions .github/workflows/twister.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,11 @@ jobs:
BSIM_COMPONENTS_PATH: /opt/bsim/components
TWISTER_COMMON: '--no-detailed-test-id --force-color --inline-logs -v -N -M --retry-failed 3 --timeout-multiplier 2 '
WEEKLY_OPTIONS: ' -M --build-only --all --show-footprint --report-filtered'
PR_OPTIONS: ' --clobber-output --integration'
PR_OPTIONS: ' --clobber-output --integration --force-platform-toolchain native_sim/native:llvm '
PUSH_OPTIONS: ' --clobber-output -M --show-footprint --report-filtered'
COMMIT_RANGE: ${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.head.sha }}
BASE_REF: ${{ github.base_ref }}
LLVM_TOOLCHAIN_PATH: /usr/lib/llvm-16
steps:
- name: Print cloud service information
run: |
Expand Down Expand Up @@ -149,11 +150,19 @@ jobs:
export ZEPHYR_BASE=${PWD}
export ZEPHYR_TOOLCHAIN_VARIANT=zephyr
python3 ./scripts/ci/test_plan.py -c origin/${BASE_REF}.. --pull-request --no-detailed-test-id
./scripts/twister --subset ${{matrix.subset}}/${{ strategy.job-total }} --load-tests testplan.json ${TWISTER_COMMON} ${PR_OPTIONS}
./scripts/twister \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do I understand correctly that:

  • now we would be running in the PR CI always with clang for native_sim[32] and never with gcc?
  • the main and weekly CI jobs would still run with gcc

If so, some thoughts, and my main concerns with this approach:

  • Having a different compiler in PRs and main, I expect trouble with breakages being discovered in main instead of PRs, and confused users about "why did the PR CI did not catch this?".
  • I expect the majority of users will keep using gcc also with native_sim, and that therefore we will be reducing needed coverage in PRs.
  • Using a different compiler in CI in native_sim, native_sim//64 and the bsim targets will also cause confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do I understand correctly that:

  • now we would be running in the PR CI always with clang for native_sim[32] and never with gcc?
  • the main and weekly CI jobs would still run with gcc

In PRs, you get the gcc coverage through different variants, i.e 64 bit and nrf_bsim. Does it really matter what toolchain you built with, the goal it to build the simulator and run tests using it, it is never about the simulator itself.

You still get gcc coverage through main branch and other workflows deploying native_sim for testing, i.e. bluetooth.

--subset ${{matrix.subset}}/${{ strategy.job-total }} \
--load-tests testplan.json \
${TWISTER_COMMON} \
${PR_OPTIONS}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

twister logs (printouts) do not tell which toolchain was used. I expect users will be confused if an error occurs in only clang or only gcc and we do not tell them neither that it was built with that compiler, nor that they need to set that option to repro the issue
(this issue existed before, but before at least clang was only used in the clang job which was a hint. If we mix them now in the same workflow it is worse).

Note the testplan.json and reports "testsuites" do not indicate the toolchain either (it is in the "environment" though)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the testplan.json and reports "testsuites" do not indicate the toolchain either (it is in the "environment" though)

well, right now it says toolchain is zephyr, althoguh for native_sim it is using the host toolchain and not using the zephyr sdk at all.

if [ "${{matrix.subset}}" = "1" -a ${{needs.twister-build-prep.outputs.fullrun}} = 'True' ]; then
./scripts/zephyr_module.py --twister-out module_tests.args
if [ -s module_tests.args ]; then
./scripts/twister +module_tests.args --outdir module_tests ${TWISTER_COMMON} ${PR_OPTIONS}
./scripts/twister \
+module_tests.args \
--outdir module_tests \
${TWISTER_COMMON} \
${PR_OPTIONS}
fi
fi

Expand Down
5 changes: 5 additions & 0 deletions scripts/pylib/twister/twisterlib/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,11 @@ def add_parse_arguments(parser = None) -> argparse.ArgumentParser:
help="Do not filter based on toolchain, use the set "
" toolchain unconditionally")

parser.add_argument("--force-platform-toolchain", action="append",
help="Force a toolchain variant for a platform in the "
"form of <platform>:<toolchain variant>. "
"(Can be specified multiple time)")

parser.add_argument("--gcov-tool", type=Path, default=None,
help="Path to the gcov tool to use for code coverage "
"reports")
Expand Down
7 changes: 6 additions & 1 deletion scripts/pylib/twister/twisterlib/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,8 +645,13 @@ def run_cmake(self, args="", filter_stages=None):
f'-D{warning_command}={warnings_as_errors}',
f'-DEXTRA_GEN_EDT_ARGS={gen_edt_args}',
f'-G{self.env.generator}',
f'-DPython3_EXECUTABLE={pathlib.Path(sys.executable).as_posix()}'
f'-DPython3_EXECUTABLE={pathlib.Path(sys.executable).as_posix()}',
]
if self.env.options.force_platform_toolchain:
for e in self.env.options.force_platform_toolchain:
platform, variant = e.split(":")
if platform and variant and self.platform.name == platform:
cmake_args.append(f'-DZEPHYR_TOOLCHAIN_VARIANT={variant}')

# If needed, run CMake using the package_helper script first, to only run
# a subset of all cmake modules. This output will be used to filter
Expand Down
1 change: 1 addition & 0 deletions scripts/tests/twister/test_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ def mock_popen(*args, **kwargs):
cmake.options.overflow_as_errors = False
cmake.env = mock.Mock()
cmake.env.generator = 'dummy_generator'
cmake.env.options.force_platform_toolchain = []

cmake_path = os.path.join('dummy', 'cmake')

Expand Down
Loading