twister: add option to create shorter build paths #41930

gopiotr · 2022-01-18T20:38:06Z

Proposition in this PR are strictly connected with problem described here: #41929

Originally when Twister is call by following command:

python scripts\twister --outdir C:\tb --ninja -p nrf5340dk_nrf5340_cpuapp -T samples/subsys/portability/cmsis_rtos_v1/timer_synchronization

It create following build folder: C:\tb\nrf5340dk_nrf5340_cpuapp\samples\subsys\portability\cmsis_rtos_v1\timer_synchronization\sample.portability.cmsis_rtos_v1.timer_synchronization which is problematic for CMake on Windows OS.

Thanks to add --short-build-path option to Twister command:

python scripts\twister --outdir C:\tb --short-build-path --ninja -p nrf5340dk_nrf5340_cpuapp -T samples/subsys/portability/cmsis_rtos_v1/timer_synchronization

Twister create originally build directory, but moreover it create also new directory with shorter path C:\tb\twister_links\test_0 and next create link between those two directories. Thank to that shorter path can be pass to CMake which allow to remove problem with too long path and at once build results can be found on originally longer path.

Without this feature I am not able to use Twister during testing Zephyr's samples and tests on Windows OS.

Fixes #41929

hakehuang · 2022-01-19T03:26:26Z

scripts/twister

@@ -584,6 +584,12 @@ structure in the main Zephyr tree: boards/<arch>/<board_name>/""")
             "This directory will be cleaned unless '--no-clean' is set. "
             "The '--clobber-output' option controls what cleaning does.")

+    parser.add_argument(


please put this argument in alpha-beta order, this is starting from s, and shall not put before o

Oh, yes, you're right, I replaced it in correct place :)

hakehuang · 2022-01-19T03:28:13Z

scripts/pylib/twister/twisterlib.py

@@ -1887,7 +1887,10 @@ class TestInstance(DisablePyTestCollectionMixin):
        out directory used is <outdir>/<platform>/<test case name>
    """

-    def __init__(self, testcase, platform, outdir):
+    # used for creating shorter build paths
+    shortcut_dir_counter = 0


it is not our practice to add a class variable, why it can not be a instant variable?

Yes, I agree with you, that this is not the best practice, but I need this variable, which works as a global counter (it is used to create unique folder name) and this value need to be shared between instances of TestInstance class. If you have better idea how to create such global variable, I am open for propositions :)

the class instance only count in one process or twister instance, if you really care about the possible conflect, maybe use filelock would be more safe

Yes, you are right, I do not predict this case when two Twister instances are run simultaneously (especially when --subset option is used). So I have to refactor this approach. Thank you for your comment!

hakehuang · 2022-01-19T03:30:14Z

scripts/pylib/twister/twisterlib.py

+        shortcut_path = os.path.join(shortcuts_dir_path, shortcut_name)
+        TestInstance.shortcut_dir_counter += 1
+
+        os.system(f"mklink /J {shortcut_path} {self.build_dir}")


this would be only works in windows.

Yes, it will, but I add check if this code is run on Windows at the beginning of this method:

def create_build_dir_shortcut(self): """ Create build directory with original "long" path. Next take shorter path (based on TestInstance class internal counter) and link them with original path - create shortcut. At the end replace build_dir to shorter path. This action helps to limit path length which can be significant during building by CMake on Windows OS. """ if os.name != "nt": # if OS is not Windows logger.warning("Creating shorter build paths available only on " "Windows OS") return

--short-build-path can be used only on Windows OS (what is described in option's help). Only on Windows OS exists problem with too long directory paths. So, I do not see any sense to extend this option to Linux OS where this problem does not exist.

PerMac · 2022-01-25T09:31:38Z

scripts/pylib/twister/twisterlib.py

+        while not shortcut_created_flag:
+            shortcut_name = self.get_new_shortcut_name(shortcuts_dir_path)
+            shortcut_path = os.path.join(shortcuts_dir_path, shortcut_name)
+            command = ["mklink", "/J", f"{shortcut_path}", f"{self.build_dir}"]
+            result = subprocess.call(command, shell=True)
+            if result == 0:
+                shortcut_created_flag = True


Don't we need any break condition to prevent infinite loops here?

I don't think running multiple twister instances is a real use case as it doesn't bring any value, since twister already handles parallelization internally. Also, each instance will create its own output dir, so if shortcuts are there, there won't be any conflict between shortcuts from multiple instances. Running multiple twister instances for subset is AFAIK used either in parallel on separate agents or on a single agent sequentially.
I am not sure if the proposed mechanism will prevent race conditions which might cause mess in links when build processes run in parallel within a single twister instance.
What do you think about using locks? E.g. the counter variable is define at the twister initialization and has getter/setter methods with a lock. When a shortcut is created, the variable is locked, the counter is increased, shortcut created, and the lock is released? Using such approach might also optimize the code a bit, since you won't need to scan the whole catalog looking for the last number and instead get it from the counter immediately. You can check here how it was implemented for counting test cases: https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/pylib/twister/twisterlib.py#L94

gmarull · 2022-01-25T14:23:53Z

scripts/pylib/twister/twisterlib.py

+        while not shortcut_created_flag:
+            shortcut_name = self.get_new_shortcut_name(shortcuts_dir_path)
+            shortcut_path = os.path.join(shortcuts_dir_path, shortcut_name)
+            command = ["mklink", "/J", f"{shortcut_path}", f"{self.build_dir}"]


why isn't os.symlink used (or pathlib symlink)?

gopiotr · 2022-01-26T15:27:09Z

With recently changes I move creating shortcuts methods to TestSuite class and I call it in twister file after performing filtration and before run CMake, building and executing. This solution do not demand any additional protection of shortcut_dir_counter variable because this fragment of code is executed before any parallelization. Moreover moving shortcut_dir_counter variable (which is used during creating shortcut names) from TestInstance to the TestSuite class resolves the problem with class variable, because TestSuite instance is created only once during all program running and is able to store this counter among all TestInstance.

why isn't os.symlink used (or pathlib symlink)?

@gmarull I tried to use os.symlink and pathlib symlink but both solutions demand Administrator privilege to create symlink. So I decided to stay with mklink raw command because I do not want to enforce on potential user to run Twister with Administrator privilege.

@PerMac proposed also idea to remove --short-build-path option and introduce creating those shortcuts by default when Twister is run on Windows OS. I think that it could be a good idea, but I would like to get to know other reviewer opinions about it :)

tejlmand

why invent yet another term, shortcut ?

Let's use link, which is a term familiar to developers, after all, that's what we are creating:

There are three types of file links supported in the NTFS file system: hard links, junctions, and symbolic links.

and

A junction (also called a soft link) differs from a hard link in that the storage objects it references are separate directories,

https://docs.microsoft.com/en-us/windows/win32/fileio/hard-links-and-junctions

and the command is name mklink

tejlmand · 2022-02-07T13:56:12Z

scripts/pylib/twister/twisterlib.py

+        if os.name != "nt":  # if OS is not Windows
+            logger.warning("Creating shorter build paths available only on "
+                           "Windows OS")
+            return


any particular reason this cannot work on other OS'es ?

On macOS / Linux we could create symlinks instead.

That will allow users to pass on --short-build-path regardless of the platform on which they are executing.

tejlmand · 2022-02-07T14:13:43Z

scripts/pylib/twister/twisterlib.py

+                           "Windows OS")
+            return
+
+        shortcuts_dir_name = "scs"  # folder for all shortcuts


I assume this suddenly means that no one can create a board named scs.

How about twister_links ?
Reasons:

twister is already used for other twister generated files, like twister.csv, twister.log, etc.
So twister can already today be considered reserved name.

_links are clearer in its intention to the reader than scs.

and twister_links are still short, compared to existing behavior.

tejlmand · 2022-02-07T14:14:59Z

scripts/pylib/twister/twisterlib.py

+        command = ["mklink", "/J", f"{shortcut_path}", f"{instance.build_dir}"]
+        subprocess.call(command, shell=True)


I think we should also support ln -s here for other OS'es.

I see no reasons why this new argument should not be supported on all platforms.

tejlmand · 2022-02-07T14:19:59Z

scripts/pylib/twister/twisterlib.py

+            if instance.status != "skipped":
+                self._create_build_dir_shortcut(shortcuts_dir_path, instance)
+
+    def _create_build_dir_shortcut(self, shortcuts_dir_path, instance):


why invent yet another term, shortcut ?

Let's use link, which is a term familiar to developers, after all, that's what we are creating:

There are three types of file links supported in the NTFS file system: hard links, junctions, and symbolic links.

and

A junction (also called a soft link) differs from a hard link in that the storage objects it references are separate directories,

https://docs.microsoft.com/en-us/windows/win32/fileio/hard-links-and-junctions

and the command is name mklink

tejlmand · 2022-02-07T14:23:03Z

One more question.
I would expect that CMake had to be invoked with the link as its build directory, or does that happen automatically ?

tejlmand

One more question. I would expect that CMake had to be invoked with the link as its build directory, or does that happen automatically ?

Guess the answer is here: #41930 (comment)

thank to that shorter path can be pass to CMake

So where in code does this happen ?
Cause I can't spot that location.

gopiotr · 2022-02-07T14:34:19Z

@tejlmand

CMake is call in Twister exactly here:

zephyr/scripts/pylib/twister/twisterlib.py

Line 2180 in 677ffc8

p = subprocess.Popen(cmd, **kwargs)

Few lines earlier the command with CMake is built and there the build directory (which is the link, when --short-build-path option is enable) is passed:

zephyr/scripts/pylib/twister/twisterlib.py

Line 2151 in 677ffc8

f'-B{self.build_dir}',

stephanosio · 2022-02-13T05:21:38Z

If we can predict / detect this situation, then printing a warning together with a hint about --short-build-path would be very nice, but I see that as extension to this PR.

Since this is a very widespread problem when running twister on Windows (thanks to how twister creates a bunch of directories with very long names), I think it would make sense to display that warning by default on Windows when --short-build-path is not specified, regardless of whether a particular run will be affected by the long path issues -- moreover, detecting such conditions would be non-trivial as it can be caused by CMake, GNU Make|Ninja, Binutils/GCC, QEMU or anything that gets invoked by the build system.

We can get rid of this behaviour when all the Windows build tools are updated to support long paths in the future.

stephanosio

LGTM

Verified working on Linux, macOS and Windows build environments:
https://github.com/stephanosio/gha-runner-zephyr-test/actions/runs/1835838884

squash! [nrf fromlist] twister: add option to create shorter build paths This changes was introduced to this open Upstream PR: zephyrproject-rtos/zephyr#41930 Signed-off-by: Piotr Golyzniak <piotr.golyzniak@nordicsemi.no>

carlescufi · 2022-02-14T14:00:53Z

LGTM

Verified working on Linux, macOS and Windows build environments: https://github.com/stephanosio/gha-runner-zephyr-test/actions/runs/1835838884

Thanks for testing @stephanosio!

carlescufi

LGTM

squash! [nrf fromlist] twister: add option to create shorter build paths This changes was introduced to this open Upstream PR: zephyrproject-rtos/zephyr#41930 Signed-off-by: Piotr Golyzniak <piotr.golyzniak@nordicsemi.no>

nashif

Does not work for me on Linux...

i9:zephyr((HEAD detached at upstream/pull/41930)): twister -T samples/hello_world/ --short-build-path
Renaming output directory to /home/nashif/Work/zephyrproject/zephyr/twister-out.2
INFO    - Zephyr version: v3.0.0-rc3-2-g5b7e539480ea
INFO    - JOBS: 20
INFO    - Using 'zephyr' toolchain.
INFO    - Selecting default platforms per test case
INFO    - Building initial testcase list...
INFO    - 1 test scenarios (39 configurations) selected, 14 configurations discarded due to filters.
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue

ERROR   - qemu_malta                samples/hello_world/sample.basic.helloworld        FAILED: Build failure
ERROR   - see: /home/nashif/Work/zephyrproject/zephyr/twister-out/qemu_malta/samples/hello_world/sample.basic.helloworld/build.log
INFO    - Total complete:    1/  25   4%  skipped:   14, failed:    1
ERROR   - qemu_nios2                samples/hello_world/sample.basic.helloworld        FAILED: Build failure
ERROR   - see: /home/nashif/Work/zephyrproject/zephyr/twister-out/qemu_nios2/samples/hello_world/sample.basic.helloworld/build.log
INFO    - Total complete:    2/  25   8%  skipped:   14, failed:    2
ERROR   - qemu_malta_be             samples/hello_world/sample.basic.helloworld        FAILED: Build failure
ERROR   - see: /home/nashif/Work/zephyrproject/zephyr/twister-out/qemu_malta_be/samples/hello_world/sample.basic.helloworld/build.log
INFO    - Total complete:    3/  25  12%  skipped:   14, failed:    3
ERROR   - m2gl025_miv               samples/hello_world/sample.basic.helloworld        FAILED: Build failure
ERROR   - see: /home/nashif/Work/zephyrproject/zephyr/twister-out/m2gl025_miv/samples/hello_world/sample.basic.helloworld/build.log
INFO    - Total complete:    4/  25  16%  skipped:   14, failed:    4
ERROR   - qemu_arc_em               samples/hello_world/sample.basic.helloworld        FAILED: Build failure
ERROR   - see: /home/nashif/Work/zephyrproject/zephyr/twister-out/qemu_arc_em/samples/hello_world/sample.basic.helloworld/build.log

error log:

-- Found assembler: /home/nashif/bin/zephyr-sdk-0.13.2/nios2-zephyr-elf/bin/nios2-zephyr-elf-gcc
-- Configuring done
-- Generating done
-- Build files have been written to: /home/nashif/Work/zephyrproject/zephyr/twister-out/twister_links/test_21
[  1%] Generating misc/generated/syscalls.json, misc/generated/struct_tags.json
[  2%] Built target parse_syscalls_target
gmake[2]: *** No rule to make target '../../../scripts/gen_kobject_list.py', needed by 'zephyr/include/generated/kobj-types-enum.h'.  Stop.
gmake[1]: *** [CMakeFiles/Makefile2:2643: zephyr/CMakeFiles/kobj_types_h_target.dir/all] Error 2
gmake: *** [Makefile:91: all] Error 2

gopiotr · 2022-02-15T12:16:00Z

Does not work for me on Linux...

Hmmmmm, yes, now I observe, that --short-build-path option works only with --ninja flag. So far I tested this option only with Ninja generator - due to the fact, that only this one generator is available on Windows OS. I have to find what is source of this problem that simple Unix Makefiles generator do not work with links.

gopiotr · 2022-02-15T16:08:19Z

@nashif I added checking if --ninja argument is passed when --short-build-path option is chosen. I added also clear information in --short-build-path description, that --ninja argument is required in this case.

I tried to find if there is any solution to pass directory link to Unix Makefiles generator, but I did not find such information. So far it seems that it is only available with Ninja generator.

nashif · 2022-02-17T01:14:15Z

scripts/pylib/twister/twisterlib.py

+        else:  # for Linux and MAC OS
+            os.symlink(instance.build_dir, link_path)
+
+        # Here symbolic link is replaced with original build directory. It will


isn't this the other way round?

original build directory is replaced with the symbolic link...

Yes, you are right - I corrected it.

nashif · 2022-02-17T01:15:08Z

@nashif I added checking if --ninja argument is passed when --short-build-path option is chosen. I added also clear information in --short-build-path description, that --ninja argument is required in this case.

ok, that works.

Add possibility to create shorter build paths when --short-build-path option is enabled. This can help during run Twister on Windows OS and building programs by CMake which has a problem when paths with building files are too long. The solution based on symbolic links which help to connect "traditional" long paths with shorter one like: "twister_links/test_0". Fixes zephyrproject-rtos#41929 Signed-off-by: Piotr Golyzniak <piotr.golyzniak@nordicsemi.no>

gopiotr requested review from nashif, hakehuang and PerMac January 18, 2022 20:38

github-actions bot added the area: Twister Twister label Jan 18, 2022

gopiotr force-pushed the pr/shorter_build_paths branch from 7f7b97d to d395d38 Compare January 18, 2022 20:47

hakehuang requested changes Jan 19, 2022

View reviewed changes

gopiotr force-pushed the pr/shorter_build_paths branch from d395d38 to 4541a07 Compare January 19, 2022 09:12

hakehuang previously approved these changes Jan 19, 2022

View reviewed changes

zephyrbot requested review from chen-png, enjiamai and galak January 19, 2022 15:27

zephyrbot assigned nashif Jan 19, 2022

gopiotr added the DNM This PR should not be merged (Do Not Merge) label Jan 19, 2022

gopiotr force-pushed the pr/shorter_build_paths branch 3 times, most recently from db1e4ed to eb7fb10 Compare January 19, 2022 20:58

carlescufi requested a review from tejlmand January 25, 2022 09:16

PerMac requested changes Jan 25, 2022

View reviewed changes

gmarull reviewed Jan 25, 2022

View reviewed changes

gopiotr force-pushed the pr/shorter_build_paths branch from eb7fb10 to fa585b0 Compare January 26, 2022 14:05

gopiotr removed the DNM This PR should not be merged (Do Not Merge) label Jan 26, 2022

gopiotr requested review from nashif and removed request for nashif January 31, 2022 14:38

PerMac self-requested a review February 1, 2022 14:27

PerMac previously approved these changes Feb 1, 2022

View reviewed changes

tejlmand requested changes Feb 7, 2022

View reviewed changes

stephanosio force-pushed the pr/shorter_build_paths branch from 419aa34 to 5b7e539 Compare February 13, 2022 05:11

stephanosio approved these changes Feb 13, 2022

View reviewed changes

tejlmand approved these changes Feb 14, 2022

View reviewed changes

carlescufi approved these changes Feb 14, 2022

View reviewed changes

carlescufi requested review from hakehuang, PerMac and gmarull February 14, 2022 14:35

PerMac approved these changes Feb 14, 2022

View reviewed changes

hakehuang approved these changes Feb 15, 2022

View reviewed changes

nashif requested changes Feb 15, 2022

View reviewed changes

gopiotr force-pushed the pr/shorter_build_paths branch from 5b7e539 to c3d655e Compare February 15, 2022 15:50

stephanosio requested a review from nashif February 17, 2022 00:51

nashif reviewed Feb 17, 2022

View reviewed changes

gopiotr force-pushed the pr/shorter_build_paths branch from c3d655e to 4856a24 Compare February 17, 2022 08:35

stephanosio requested a review from nashif February 17, 2022 11:16

nashif approved these changes Feb 17, 2022

View reviewed changes

stephanosio added this to the v3.1.0 milestone Feb 21, 2022

nashif merged commit b618f4b into zephyrproject-rtos:main Feb 22, 2022

stephanosio mentioned this pull request Jun 27, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

twister: add option to create shorter build paths #41930

twister: add option to create shorter build paths #41930

gopiotr commented Jan 18, 2022 •

edited

Loading

hakehuang Jan 19, 2022

gopiotr Jan 19, 2022

hakehuang Jan 19, 2022

gopiotr Jan 19, 2022

hakehuang Jan 19, 2022

gopiotr Jan 19, 2022

hakehuang Jan 19, 2022

gopiotr Jan 19, 2022 •

edited

Loading

PerMac Jan 25, 2022

PerMac Jan 25, 2022

gmarull Jan 25, 2022

gopiotr commented Jan 26, 2022

tejlmand left a comment

tejlmand Feb 7, 2022

tejlmand Feb 7, 2022

tejlmand Feb 7, 2022

tejlmand Feb 7, 2022

tejlmand commented Feb 7, 2022

tejlmand left a comment

gopiotr commented Feb 7, 2022

stephanosio commented Feb 13, 2022 •

edited

Loading

stephanosio left a comment

carlescufi commented Feb 14, 2022

carlescufi left a comment

nashif left a comment •

edited

Loading

gopiotr commented Feb 15, 2022 •

edited

Loading

gopiotr commented Feb 15, 2022

nashif Feb 17, 2022

gopiotr Feb 17, 2022

nashif commented Feb 17, 2022

		command = ["mklink", "/J", f"{shortcut_path}", f"{instance.build_dir}"]
		subprocess.call(command, shell=True)

twister: add option to create shorter build paths #41930

twister: add option to create shorter build paths #41930

Conversation

gopiotr commented Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gopiotr Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gopiotr commented Jan 26, 2022

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand commented Feb 7, 2022

tejlmand left a comment

Choose a reason for hiding this comment

gopiotr commented Feb 7, 2022

stephanosio commented Feb 13, 2022 • edited Loading

stephanosio left a comment

Choose a reason for hiding this comment

carlescufi commented Feb 14, 2022

carlescufi left a comment

Choose a reason for hiding this comment

nashif left a comment • edited Loading

Choose a reason for hiding this comment

gopiotr commented Feb 15, 2022 • edited Loading

gopiotr commented Feb 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nashif commented Feb 17, 2022

gopiotr commented Jan 18, 2022 •

edited

Loading

gopiotr Jan 19, 2022 •

edited

Loading

stephanosio commented Feb 13, 2022 •

edited

Loading

nashif left a comment •

edited

Loading

gopiotr commented Feb 15, 2022 •

edited

Loading