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

Fixed external library linking for Clang in VS2019+ #1522

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Fixed external library linking for Clang in VS2019+ #1522

merged 1 commit into from
Sep 23, 2020

Conversation

nickclark2016
Copy link
Member

What does this PR do?

Fixes problems referenced in #1520 and #1507 where the clang toolset produces a bad external library path when used with VS2019. Current behavior deferred the library generation to gcc's implementation, such that external libraries received the string "-lName" (e.g. "-lSDL") instead of "name.lib" (e.g. "SDL.lib").

How does this PR change Premake's behavior?

Changes the clang tool's getlinks method to defer to MSC when the action is VS2019 or newer.

Anything else we should know?

N/A

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

I appreciate the effort, but VS2019 supports more than just Windows with Clang.

@@ -282,7 +283,12 @@
--

function clang.getlinks(cfg, systemonly, nogroups)
return gcc.getlinks(cfg, systemonly, nogroups)
-- For visual studio projects, the links should match the MSC convention
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, it should only apply to Windows projects, not all projects. Furthermore, this change should be made in the VS module.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can easily move the change to the VS module. Just so I go the agreed course of action, would you say:

  • Changes made in the vs2019 module
  • Changes only affect the clang and msc toolsets for windows projects

@nickclark2016
Copy link
Member Author

Not sure why the test is failing for macosx release on the response code test, especially since what I did shouldn't have affected that

@nickclark2016
Copy link
Member Author

Huh, wonder what the issue with the last run was. Glad to see the release macosx build is working now, though.

@samsinsane
Copy link
Member

The HTTP unit tests connect to an external service, it's likely it was just a weird issue within the Azure network - I get this a lot at work with Azure Pipelines.

@samsinsane samsinsane merged commit 6b545b9 into premake:master Sep 23, 2020
@nickclark2016
Copy link
Member Author

The HTTP unit tests connect to an external service, it's likely it was just a weird issue within the Azure network - I get this a lot at work with Azure Pipelines.

Makes sense. Thanks for closing this out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants