Skip to content

Conversation

@gBillal
Copy link
Contributor

@gBillal gBillal commented Dec 26, 2025

In this PR we fix the issue of linking libraries declared in the deps config.

Also I deleted the instruction that deletes the whole folder before starting compilation because it deletes any compiled natives/libs before the current one (if they're in the same directory or on a sub-directory)

@gBillal gBillal requested a review from FelonEkonom as a code owner December 26, 2025 15:59
@FelonEkonom FelonEkonom requested review from Noarkhh and varsill January 8, 2026 16:39
@FelonEkonom FelonEkonom moved this to In Review in Smackore Jan 8, 2026
@FelonEkonom FelonEkonom self-assigned this Jan 8, 2026
@FelonEkonom FelonEkonom requested review from Copilot and mat-hek and removed request for Noarkhh January 13, 2026 15:09
@FelonEkonom FelonEkonom removed the request for review from varsill January 13, 2026 15:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a missing feature in the Visual Studio toolchain where dependencies declared in the deps configuration were not being linked during compilation. It also improves the directory handling to prevent deletion of previously compiled artifacts.

Changes:

  • Added logic to extract and format dependency library paths for MSVC linking commands
  • Modified directory setup from recursive deletion and recreation to conditional creation only
  • Introduced helper functions paths/2 and path/1 to handle path formatting and quoting

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

[
"(cl #{compile_options} #{includes_part} #{sources_part})",
~s[(lib /OUT:"#{PathHelper.fix_slashes(output_path)}.lib" *.obj)]
~s[(lib /OUT:"#{PathHelper.fix_slashes(output_path)}.lib" *.obj #{deps})]
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

Static libraries (.lib files created by the lib tool) are archives of object files and typically don't link other libraries. The lib command may not properly handle the deps parameter. In the Unix implementation (common/unix.ex, line 74-76), the ar command for :lib type natives intentionally does not include deps, as dependencies are resolved when the static library is later linked into an executable or shared library. Consider removing deps from this lib command to match the Unix behavior.

Suggested change
~s[(lib /OUT:"#{PathHelper.fix_slashes(output_path)}.lib" *.obj #{deps})]
~s[(lib /OUT:"#{PathHelper.fix_slashes(output_path)}.lib" *.obj)]

Copilot uses AI. Check for mistakes.
end

defp path(path) do
path = path |> String.replace(~S("), ~S(\")) |> Path.expand()
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The path function uses Path.expand() and adds quotes but doesn't apply PathHelper.fix_slashes() to convert forward slashes to backslashes for Windows. This is inconsistent with how other paths are handled in this file (see lines 35, 39, 52, 72, 84) where PathHelper.fix_slashes() is used. The Unix implementation also uses Path.expand() in its path function, but that's appropriate for Unix systems. On Windows, the dependency paths may contain forward slashes that need to be converted to backslashes for MSVC tools. Consider adding PathHelper.fix_slashes() to the path function or applying it to the deps before passing them to paths().

Suggested change
path = path |> String.replace(~S("), ~S(\")) |> Path.expand()
path =
path
|> String.replace(~S("), ~S(\"))
|> Path.expand()
|> PathHelper.fix_slashes()

Copilot uses AI. Check for mistakes.
@mat-hek mat-hek merged commit 8f32a94 into membraneframework:master Jan 13, 2026
7 of 10 checks passed
@github-project-automation github-project-automation bot moved this from In Review to Done in Smackore Jan 13, 2026
@gBillal gBillal deleted the visual-studio-include-deps branch January 13, 2026 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants