524 bug 0143b7 #525

ramseymcgrath · 2026-01-03T08:23:07Z

No description provided.

- Fixed nested src/src/ directory structure that was causing duplicate module definitions - When copying source files, don't append 'src' to output_dir since relative paths already include it - This resolves Vivado synthesis errors about overwriting module definitions and unresolved hierarchical names - Fixes issue where files were being added twice to Vivado project (once from src/ and once from src/src/) Root cause: _copy_source_files was preserving directory structure from board_path, which included 'src/', but was also being given an output_dir of 'board_output_dir/src', resulting in files being placed at 'board_output_dir/src/src/file.sv'.

- test_copy_source_files_no_nested_src_directory: Verifies files aren't copied to src/src/ - test_unified_build_script_no_duplicate_source_files: Ensures build script doesn't add duplicate files These tests prevent regression of GitHub issue #524 where duplicate module definitions were causing Vivado synthesis to fail due to nested src/src/ directory structure.

github-actions · 2026-01-03T08:25:48Z

E2E Integration Test Summary

Workflow Run: 767
Commit: c476e20
Branch: 525/merge
Triggered by: pull_request
Timestamp: Sat Jan 3 08:25:47 UTC 2026

Test Results

End-to-End Tests

❌ E2E Tests: FAILED
⏸️ Security Scan: SKIPPED
⏸️ Performance Analysis: SKIPPED

Artifacts

Test reports and logs available in workflow artifacts
Artifacts retained for 7-30 days depending on type

Copilot

Pull request overview

This pull request fixes a bug (GitHub Issue #524) where source files were being copied to a nested src/src/ directory structure instead of a flat src/ structure, causing duplicate module definition errors in Vivado builds.

Key Changes:

Refactored _copy_source_files to use a flat file structure, placing all source files directly in output_dir/src/ regardless of repository structure
Removed directory structure preservation logic that was creating nested src/src/ paths
Added comprehensive regression tests to prevent nested directory issues and verify flat structure behavior

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
`src/vivado_handling/pcileech_build_integration.py`	Refactored `_copy_source_files` to implement flat directory structure (all files go to `output_dir/src/`), removed `dest_path.parent.mkdir` call, and added detailed documentation explaining the design principle
`tests/test_pcileech_build_integration.py`	Updated existing tests for flat structure behavior, added 6 new test cases covering nested directory prevention, filename collisions, manifest tracker integration, empty file lists, IO error handling, and TCL script deduplication

Comments suppressed due to low confidence (6)

tests/test_pcileech_build_integration.py:601

Variable script_path is not used.

            script_path = integration.create_unified_build_script("artix7")

tests/test_pcileech_build_integration.py:633

Variable script_path is not used.

            script_path = integration.create_unified_build_script("artix7")

tests/test_pcileech_build_integration.py:663

Variable script_path is not used.

            script_path = integration.create_unified_build_script("artix7")

tests/test_pcileech_build_integration.py:690

Variable script_path is not used.

            script_path = integration.create_unified_build_script("pcileech_100t484_x1")

tests/test_pcileech_build_integration.py:734

Variable script_path is not used.

            script_path = integration.create_unified_build_script(board_name)

tests/test_pcileech_build_integration.py:844

Variable result is not used.

            result = integrate_pcileech_build("artix7", self.output_dir, device_config)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-03T08:25:57Z

src/vivado_handling/pcileech_build_integration.py

+        # Copy source files from repository  
+        # Don't append "src" to output_dir as it will be preserved from the board path


The comment says "Don't append 'src' to output_dir as it will be preserved from the board path", but the new implementation actually does the opposite - it explicitly appends 'src' internally within the _copy_source_files method. The comment should be clarified to say something like "Pass base output_dir directly; the method will create and use output_dir/src/ internally" to accurately reflect the implementation.

Suggested change

# Copy source files from repository

# Don't append "src" to output_dir as it will be preserved from the board path

# Copy source files from repository.

# Pass base board_output_dir; _copy_source_files will create and use board_output_dir / "src" internally.

Copilot · 2026-01-03T08:25:57Z

tests/test_pcileech_build_integration.py

+            src_count = str(result_path).count('/src/')
+            self.assertEqual(src_count, 1, 
+                           f"Path should have exactly one 'src' directory, found {src_count}: {result_path}")


The test counts occurrences of '/src/' in the path string, which will fail on Windows systems where paths use backslashes. Consider using Path(result_path).parts and counting 'src' occurrences in the parts list instead, which is platform-independent.

Suggested change

src_count = str(result_path).count('/src/')

self.assertEqual(src_count, 1,

f"Path should have exactly one 'src' directory, found {src_count}: {result_path}")

path_parts = Path(result_path).parts

src_count = sum(1 for part in path_parts if part == "src")

self.assertEqual(

src_count,

1,

f"Path should have exactly one 'src' directory, found {src_count}: {result_path}",

)

Copilot · 2026-01-03T08:25:58Z

tests/test_pcileech_build_integration.py

+        # Both files should be copied (last one overwrites)
+        self.assertEqual(len(result), 2)
+
+        # Both should target the same destination path
+        expected_dest = board_output_dir / "src" / "common.sv"
+        for call_args in mock_copy2.call_args_list:
+            dest_path = call_args[0][1]
+            self.assertEqual(dest_path, expected_dest,
+                           "All files with same name should map to same destination")


The test expects all files with the same name to be copied to the same destination path (line 310-311), but the implementation actually allows this and just overwrites. The assertion on line 304 checks len(result) == 2, which is correct, but the comment on line 303 says "Both files should be copied (last one overwrites)" which contradicts the test behavior. The test should verify that both copy operations were attempted, not that the result list contains 2 items with the same destination.

Suggested change

# Both files should be copied (last one overwrites)

self.assertEqual(len(result), 2)

# Both should target the same destination path

expected_dest = board_output_dir / "src" / "common.sv"

for call_args in mock_copy2.call_args_list:

dest_path = call_args[0][1]

self.assertEqual(dest_path, expected_dest,

"All files with same name should map to same destination")

# Both files should be copied (two copy operations; last one overwrites)

self.assertEqual(len(result), 2)

self.assertEqual(mock_copy2.call_count, 2)

# Both copy operations should target the same destination path

expected_dest = board_output_dir / "src" / "common.sv"

for call_args in mock_copy2.call_args_list:

dest_path = call_args[0][1]

self.assertEqual(

dest_path,

expected_dest,

"All files with same name should map to same destination",

)

Copilot · 2026-01-03T08:25:58Z

tests/test_pcileech_build_integration.py

+    def test_copy_source_files_with_manifest_tracker_duplicate_prevention(self, mock_copy2):
+        """Test that manifest tracker prevents actual duplicate file copies."""
+        # Create a mock manifest tracker that rejects the second file
+        mock_manifest = MagicMock()
+        mock_manifest.add_copy_operation.side_effect = [True, False]  # Accept first, reject second
+
+        integration = PCILeechBuildIntegration(
+            self.output_dir, 
+            self.repo_root,
+            manifest_tracker=mock_manifest
+        )
+
+        src_files = [
+            Path("/tmp/test_repo/boards/board1/src/file1.sv"),
+            Path("/tmp/test_repo/boards/board1/src/file2.sv"),
+        ]
+        self.mock_template_discovery.get_source_files.return_value = src_files
+        self.mock_template_discovery.get_pcileech_core_files.return_value = {}
+
+        result = integration._copy_source_files("board1", Path("/tmp/output/board1"))
+
+        # Only first file should be copied (second rejected by manifest)
+        self.assertEqual(len(result), 1)
+        self.assertEqual(mock_copy2.call_count, 1)
+        self.assertEqual(result[0].name, "file1.sv")


The test documentation says "Test that manifest tracker prevents actual duplicate file copies" but the implementation doesn't check if copy2 was called only once. If manifest tracker rejects the second file, shutil.copy2 should only be called once, but the test only verifies the result length and doesn't assert that mock_copy2.call_count equals 1. Add an assertion to verify that copy2 is only invoked once when manifest tracker rejects duplicates.

Copilot · 2026-01-03T08:25:58Z

tests/test_pcileech_build_integration.py

+        })
+
+        # Call create_unified_build_script and capture the script content
+        script_path = integration.create_unified_build_script("pcileech_100t484_x1")


Variable script_path is not used.

Suggested change

script_path = integration.create_unified_build_script("pcileech_100t484_x1")

integration.create_unified_build_script("pcileech_100t484_x1")

ramseymcgrath added 3 commits December 31, 2025 19:17

..

2b8c84c

Copilot AI review requested due to automatic review settings January 3, 2026 08:23

ramseymcgrath linked an issue Jan 3, 2026 that may be closed by this pull request

[BUG] 0.14.3b7 #524

Closed

ramseymcgrath merged commit 578dcf4 into main Jan 3, 2026
11 of 22 checks passed

ramseymcgrath deleted the 524-bug-0143b7 branch January 3, 2026 08:23

Copilot started reviewing on behalf of ramseymcgrath January 3, 2026 08:23 View session

Copilot AI reviewed Jan 3, 2026

View reviewed changes

EliteCodexer mentioned this pull request Jan 4, 2026

[BUG] v0.14.3b8 - 100t builds fail and always have #527

Closed

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

524 bug 0143b7 #525

524 bug 0143b7 #525

Uh oh!

ramseymcgrath commented Jan 3, 2026

Uh oh!

Uh oh!

github-actions bot commented Jan 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Copilot AI Jan 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		# Copy source files from repository
		# Don't append "src" to output_dir as it will be preserved from the board path

	script_path = integration.create_unified_build_script("pcileech_100t484_x1")
	integration.create_unified_build_script("pcileech_100t484_x1")

524 bug 0143b7 #525

524 bug 0143b7 #525

Uh oh!

Conversation

ramseymcgrath commented Jan 3, 2026

Uh oh!

Uh oh!

github-actions bot commented Jan 3, 2026

E2E Integration Test Summary

Test Results

End-to-End Tests

Artifacts

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants