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

Inline and delete runtime.$(os).$(proj).props files #37254

Merged
merged 1 commit into from
Jun 1, 2020
Merged

Inline and delete runtime.$(os).$(proj).props files #37254

merged 1 commit into from
Jun 1, 2020

Conversation

am11
Copy link
Member

@am11 am11 commented Jun 1, 2020

Fixes #37241.

@ghost
Copy link

ghost commented Jun 1, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@am11
Copy link
Member Author

am11 commented Jun 1, 2020

cc @jkotas, something like this?

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@dagood dagood removed their request for review June 1, 2020 18:43
@dagood
Copy link
Member

dagood commented Jun 1, 2020

This is something I've wanted to investigate doing for a while, but never had a reason to--now that we're adding more platforms, good time. I'm not aware of any reasons not to do this (but would rather leave in-depth review to others more involved with these changes). 👍

@am11
Copy link
Member Author

am11 commented Jun 1, 2020

CoreCLR Pri0 Runtime Tests Run Windows_NT arm checked is unrelated to PR changes, related issue: #37132.

@jkotas jkotas merged commit 7f3cdea into dotnet:master Jun 1, 2020
@am11 am11 deleted the feature/cleanup branch June 1, 2020 20:42
@AaronRobinsonMSFT
Copy link
Member

@am11 I believe this has broken shipping the static lib on Windows #35616 (comment).

@am11
Copy link
Member Author

am11 commented Jun 14, 2020

We were including libnethost.lib as well as nethost.lib on Windows before this change. This PR generalizes it and only includes nethost.lib via this pattern:

<NativeBinary Include="$(DotNetHostBinDir)/$(LibraryFilePrefix)nethost$(StaticLibraryFileExtension)" />

(on Windows LibraryFilePrefix is empty)

do we need both files or is nethost.lib completely different?

@AaronRobinsonMSFT
Copy link
Member

@am11 Yes we need both. This is a common issue on Windows since both a static lib and import lib use the same extension (i.e. *.lib). In this case, nethost.lib is the import lib for consuming nethost.dll and libnethost.lib is the static version of the nethost binary. I would imagine that on non-Windows this mistake wasn't made.

@am11
Copy link
Member Author

am11 commented Jun 14, 2020

OK, i have added it in #37878. Yes, before it was:

Windows
-    <ArchitectureSpecificNativeFile Include="$(DotNetHostBinDir)/libnethost.lib" />
-    <ArchitectureSpecificNativeFile Include="$(DotNetHostBinDir)/nethost.lib" />
non-Windows
-    <ArchitectureSpecificNativeFile Include="$(DotNetHostBinDir)/libnethost.a" />

this change only effects Windows case, and only nethost had this specific case. ijwhost.lib and comhost.lib etc. had only one variant, without lib prefix.

@AaronRobinsonMSFT
Copy link
Member

this change only effects Windows case, and only nethost had this specific case. ijwhost.lib and comhost.lib etc. had only one variant, without lib prefix.

Yes, that is true. We only ship a static lib for the nethost scenario.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate .props files in packages directories
5 participants