[cmake] Use source_group TREE on source_group_by_folder

[CastagnaIT]

Description

Motivation and context

from VS2026 with recent cmake versions (more likely from 4.3+) the folder hierarchy on solution tree view is broken and show a full list of files without folders

How has this been tested?

Screenshots (if appropriate):

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

Checklist:

• I have read the Contributing document
• My code follows the Code Guidelines of this project
• My change requires a change to the Wiki documentation
• I have updated the documentation accordingly

[github-actions]

Review

Must Fix — list(POP_FRONT files) is fragile

The gen-expr skip relies on $< sorting before all alphanumeric chars (ASCII 36 < 65), so gen-exprs end up at the front of the sorted list and POP_FRONT pops the right element. This is an undocumented assumption. Any filename starting with a character with ASCII value < 36 (e.g. \!, #, %) would sort before $<, causing the first source_group(TREE ... FILES ${files}) call on a directory-change to still contain the gen-expr — CMake error or wrong grouping.

Correct fix: filter before the loop, drop POP_FRONT:

list(FILTER files EXCLUDE REGEX "^\\$<")

---

Medium — Loop structure unnecessary for TREE mode

source_group(TREE ...) infers the full hierarchy in one call. The per-directory loop with last_dir tracking is leftover from the old string-group approach. The body can be simplified to:

list(FILTER files EXCLUDE REGEX "^\\$<")
if(files)
  source_group(TREE "${relative_dir}" FILES ${files})
endif()

[github-actions]

list(POP_FRONT files) only works because $< (ASCII 36) sorts before letters, putting gen-exprs at the front of the sorted list. It's fragile — any filename starting with a char < ASCII 36 (!, #, %, etc.) would sort first, and POP_FRONT would then remove a legitimate source file instead.

Filter gen-exprs from files before the loop and drop this line entirely:

Suggested change

	list(POP_FRONT files)
	continue()

Add before the foreach:

list(FILTER files EXCLUDE REGEX "^\\$<")

continue() alone is sufficient since the gen-expr is never appended to files when we skip the rest of the loop body.

[CastagnaIT]

i will not apply suggested code changes to keep consistency with main xbmc project

[kodiai]

Kodiai Review Summary

What Changed

This PR updates the CMake source_group_by_folder function to use source_group(TREE ...) instead of source_group("${last_dir}" ...) to fix broken folder hierarchy in VS2026 with recent CMake versions.

Reviewed: core logic

Strengths

• ✅ Correctly identifies and skips generator expressions that would break the folder grouping logic (Helpers.cmake:30)
• ✅ Addresses the root cause by switching to the TREE option which is the modern CMake approach for hierarchical source grouping (Helpers.cmake:42, 50)

Observations

Impact

[MAJOR] Helpers.cmake (31): Incorrect list manipulation during iteration
The list(POP_FRONT files) modifies the files variable while iterating over it. Although foreach uses a snapshot (so iteration continues correctly), this line is problematic because: (1) it has no effect on the iteration itself, (2) the files variable is reused as an accumulator later in the loop (cleared at line 44, appended at line 46), so POP_FRONT may corrupt the accumulator if triggered after the first directory group is processed, (3) the continue() statement alone is sufficient to skip the generator expression.

[MAJOR] Helpers.cmake (42, 50): Non-standard usage pattern for source_group TREE
The source_group(TREE ...) command is called multiple times with directory-grouped file subsets. The standard CMake pattern is to call source_group(TREE root FILES ...) once with all files, which automatically creates the complete folder hierarchy. The current implementation groups files by directory first (line 40 condition), then calls source_group(TREE ...) for each group. While this may work, it's unconventional and adds unnecessary complexity. The proper approach would be to filter generator expressions into a separate list, then call source_group(TREE "${relative_dir}" FILES ${filtered_files}) once outside the directory-grouping loop.

Suggestions

• Optional: Refactor the entire function to eliminate the directory-grouping loop since source_group(TREE ...) handles hierarchy automatically
• Optional: Use a separate variable name for the file accumulator to avoid confusion with the input files list

Verdict

🔴 Address before merging -- 2 blocking issue(s) found

Review Details

• Files reviewed: 1

• Findings: 0 critical, 4 major, 0 medium, 0 minor (includes 2 from summary observations)

• Lines changed: +7 -2

• Profile: strict (auto, lines changed: 9)

• Contributor experience: profile-backed (using linked contributor profile guidance)

• Review completed: 2026-04-20T15:49:35.318Z

• Total wall-clock: 11m 53s

• Phase timings:

  • queue wait: 6m 28s
  • workspace preparation: 1.3s
  • retrieval/context assembly: 2.3s
  • executor handoff: 4.0s
  • remote runtime: 5m 14s
  • publication: 1.2s

• Tokens: 44 in / 15,054 out | 0.3349

• Keyword parsing: No keywords detected

[kodiai]

[MAJOR] Incorrect list manipulation during iteration.

The list(POP_FRONT files) command modifies the files variable while the foreach(file ${files}) loop is iterating over it. While foreach iterates over a snapshot (so this doesn't break the iteration), this line is problematic because:

1. It has no effect on which files are processed (the loop continues with all files)
2. The files variable is reused as an accumulator later in the loop (line 44 clears it, line 46 appends to it)
3. If a generator expression appears after the first directory group is processed, this would pop from the accumulator instead of the original list, corrupting the results
4. The continue() statement alone is sufficient to skip the generator expression

Suggested change

	list(POP_FRONT files)
	if(file MATCHES "^\\$<")
	continue()
	endif()

[kodiai]

[MAJOR] Non-standard usage pattern for source_group(TREE ...).

The source_group(TREE <root> FILES ...) command is designed to be called once with all source files to automatically create the complete folder hierarchy. The current implementation calls it multiple times with directory-grouped subsets (triggered by the if(NOT dir STREQUAL "${last_dir}") condition at line 40).

While multiple calls may work, this approach is:

• Unconventional and more complex than necessary
• Inconsistent with CMake documentation examples
• Potentially fragile if CMake's behavior changes

Standard pattern:

# Filter generator expressions
set(filtered_files "")
foreach(file ${files})
  if(NOT file MATCHES "^\\$<")
    list(APPEND filtered_files "${file}")
  endif()
endforeach()

# Create hierarchy in one call
if(filtered_files)
  source_group(TREE "${relative_dir}" FILES ${filtered_files})
endif()

This would eliminate the entire directory-grouping loop (lines 29-48) since TREE handles the hierarchy automatically.