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.

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

scripts: twister: Fix Twister Unit tests on Windows #74651

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LukaszMrugala
Copy link
Collaborator

Unit tests were failing on Windows, indicating that current Twister code is not truly multiplatform.

Problems detected:

  • Problem: Paths were deemed the main culprit. Windows has a limit of 260 character file paths by default.
    • Fix: This commit introduces a new TPath, that aims to fix the Windows limitation of supporting only up to 260 char paths by default via a path prefix disabling this limitation.
  • Problem: Windows and Linux have a limit of 260 character filepath element.
    • Fix: TestSuite name formation was updated, so it wouldn't create extremely long path elements as its name, given that TestSuite name is used in dir creation.
  • Problem: os.kill with a Linux-specific signal is used.
    • Fix: Process is used instead.
  • Problem: pathlib's WindowPaths were not serialised correctly in the reports.
    • Fix: PosixPath serialising fix was extended to other path classes.

Unit tests were adjusted (and some disabled, as they didn't make sense on a given OS).

@LukaszMrugala LukaszMrugala force-pushed the lmrugalx_twister_ut_windows branch 6 times, most recently from dd3865f to c4cc343 Compare June 24, 2024 13:43
@LukaszMrugala LukaszMrugala marked this pull request as ready for review June 24, 2024 14:43
@zephyrbot zephyrbot added the area: Twister Twister label Jun 24, 2024
@LukaszMrugala LukaszMrugala added the bug The issue is a bug, or the PR is fixing a bug label Jun 25, 2024
@LukaszMrugala LukaszMrugala added the DNM This PR should not be merged (Do Not Merge) label Jun 27, 2024
Comment on lines +28 to +31
# On Windows, without this prefix, there is 260-character path length limit.
if not normalised.startswith('\\\\?\\'):
normalised = '\\\\?\\' + normalised
return Path(normalised)
Copy link
Member

Choose a reason for hiding this comment

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

Problem: Paths were deemed the main culprit. Windows has a limit of 260 character file paths by default.
Problem: Windows and Linux have a limit of 260 character filepath element.

So you are basically trying to solve the same problem that --short-build-path (see #41930) already solves, but in a different way.

Does this actually work? Because Ninja and Zephyr SDK Windows toolchain binaries are not exactly long-path friendly, relying on the "ANSI version" of the I/O functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've had to make some fixes before answering you (Zephyr test failures). Currently, this change gives us a PASS on all Zephyr tests and all Unit Tests relevant for Windows (So without i.a. jobserver and QEMUHandler). I've tested that on a private CI.

@LukaszMrugala LukaszMrugala force-pushed the lmrugalx_twister_ut_windows branch 2 times, most recently from cc8a8d5 to 9d9fa48 Compare June 28, 2024 07:31
@LukaszMrugala LukaszMrugala force-pushed the lmrugalx_twister_ut_windows branch 8 times, most recently from 995d7f1 to f5961bc Compare July 2, 2024 14:54
@LukaszMrugala LukaszMrugala removed DNM This PR should not be merged (Do Not Merge) area: Continuous Integration labels Jul 3, 2024
KamilxPaszkiet
KamilxPaszkiet previously approved these changes Jul 9, 2024
Copy link
Collaborator

@KamilxPaszkiet KamilxPaszkiet left a comment

Choose a reason for hiding this comment

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

I tested these changes on Win 10. Everything seems fine to me.

@PerMac
Copy link
Member

PerMac commented Aug 22, 2024

It's hard for me to evaluate this PR. I don't understand the need for all this changes to paths and in particular the new TPath class. IMO TPath adds another layer which can complicate simple operations that were done with paths. I am aware about the first 2 bullet points (char limit on Windows) but I was sure we managed to resolve it with introduction of --short-build-path. In Nordic we have a CI which is building tests/samples on Windows with twister and it works. I think I also had to add --no-detailed-test-id at some point to remove paths from test ids but cannot recall what was the root cause for this.

@LukaszMrugala LukaszMrugala added the DNM This PR should not be merged (Do Not Merge) label Aug 28, 2024
@LukaszMrugala LukaszMrugala changed the title scripts: twister: Fix Twister for Windows scripts: twister: Fix Twister Unit tests on Windows Sep 4, 2024
@LukaszMrugala LukaszMrugala force-pushed the lmrugalx_twister_ut_windows branch 3 times, most recently from ae275a7 to cb24d66 Compare September 4, 2024 13:48
@LukaszMrugala LukaszMrugala force-pushed the lmrugalx_twister_ut_windows branch 3 times, most recently from 03a2e2b to fd746d8 Compare September 30, 2024 14:03
@LukaszMrugala LukaszMrugala removed the DNM This PR should not be merged (Do Not Merge) label Oct 2, 2024
@LukaszMrugala
Copy link
Collaborator Author

LukaszMrugala commented Oct 2, 2024

I have gotten back to this PR now and am able to remove the DNM label.

While Twister itself can currently be run on Windows with the use of specific Twister flag, how can someone making a change to Twister know that their change is not breaking for Windows? To verify that, we ought to have a working CI which built and runs Twister tests for Windows and does a check on PRs which change Twister in any way.

We do not have such a workflow here, but I've conducted a few test workflow run in a private repo. All unit tests pass on both Windows and Linux after those changes. Without them, unit tests fail on Windows. With Unit Tests failing on Windows, we cannot have a Windows-specific CI runs and thus cannot in any way safeguard future Windows correctness.

This is not a singular PR, but a step on the way towards better Windows support.

Unit tests were failing on Windows, indicating that current Twister
code is not truly multiplatform. Linux-specific code parts were
changed into multiplatform ones.

Paths were deemed the main culprit,
so this commit introduces a new TPath, that aims to fix the Windows
limitation of supporting only up to 260 char paths by default.

Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Twister Twister bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants