Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@tarekgh
Copy link
Member

@tarekgh tarekgh commented Jan 13, 2016

No description provided.

@tarekgh
Copy link
Member Author

tarekgh commented Jan 13, 2016

@stephentoub @ellismg @jkotas could you please have a look?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why it cannot be just called System.Native.a (without the Static part)?

Copy link
Member Author

Choose a reason for hiding this comment

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

if you called it System.Native.a then the output file will be named System.Native.a.a as the build append the extension.
Also I couldn't use System.Native name because it complained it is already used with shared library

we can use .a instead of static with keeping all other changes. this should work

@jkotas
Copy link
Member

jkotas commented Jan 13, 2016

I assume that packaging will come separately. Any thoughts on that?

@tarekgh
Copy link
Member Author

tarekgh commented Jan 13, 2016

@jkotas yes the packaging change will come after we merge the change. this change will be done by Chris Costa. I already talked to him about that.

@tarekgh
Copy link
Member Author

tarekgh commented Jan 14, 2016

CC @chcosta

@jkotas
Copy link
Member

jkotas commented Jan 14, 2016

I did not meant to name the final output System.Native.a.a - it is worse name than System.Native.Static.a

@tarekgh
Copy link
Member Author

tarekgh commented Jan 14, 2016

@jkotas yes I understand. with the current changes, the file name will be System.Native.a

the line
SET_TARGET_PROPERTIES(System.Native.a PROPERTIES OUTPUT_NAME System.Native CLEAN_DIRECT_OUTPUT 1)
is doing the renaming trick to the correct name we need

@jkotas
Copy link
Member

jkotas commented Jan 14, 2016

doing the renaming trick to the correct name we need

I see. So System.Native.a is just a local target name. In that case, I would call the local target System.Native-s to avoid confusion - -s seems to be one of the convention people are using in this situation.

Copy link
Member

Choose a reason for hiding this comment

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

We should disable the prefix one level up - same way as it is done for shared libraries.

Copy link
Member Author

Choose a reason for hiding this comment

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

who disabled the prefix on the shared libraries? I am asking because it looks not having the prefix can cause some issues. I am seeing the following quote on the SO thread

You can name one any way you want, but ld's -l assuming a lib prefix applies to both static and shared libraries and goes back a long way

so it looks to me it is better to keep the prefix for both static and dynamic libraries.

Copy link
Member

Choose a reason for hiding this comment

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

It was done in the initial commit of the Unix shims #2445 . There was some discussion about it, but I am not able to find it.

@jkotas
Copy link
Member

jkotas commented Jan 14, 2016

LGTM

@tarekgh
Copy link
Member Author

tarekgh commented Jan 14, 2016

@davidsh it looks one sockets tests failed. could you have a look?

http://dotnet-ci.cloudapp.net/job/dotnet_corefx/job/windows_nt_release_prtest/956/testReport/junit/System.Net.Sockets.Tests/SendReceive/SendToRecvFromAsync_UdpClient_Single_Datagram_UDP_IPv4/

MESSAGE:
Assert.True() Failure\r\nExpected: True\r\nActual: False
+++++++++++++++++++
STACK TRACE:
at System.Net.Sockets.Tests.SendReceive.SendToRecvFromAsync_UdpClient_Datagram_UDP(IPAddress leftAddress, IPAddress rightAddress) in d:\j\workspace\dotnet_corefx\windows_nt_release_prtest\src\System.Net.Sockets\tests\FunctionalTests\SendReceive.cs:line 192

@tarekgh
Copy link
Member Author

tarekgh commented Jan 14, 2016

@jkotas disabling the prefix globally will cause some other errors like the following. so I think either we need to disable it for System.Native only or we just keep the prefix so the library would be libSystem.Native.a

/usr/bin/ld: cannot find -llibz
/usr/bin/ld/usr/bin/ld: cannot find : cannot find -llibcrypto
-llibcurl
/usr/bin/ld: cannot find -llibssl

@jkotas
Copy link
Member

jkotas commented Jan 14, 2016

Either way is fine with me.

@tarekgh
Copy link
Member Author

tarekgh commented Jan 14, 2016

Thanks @jkotas by the way I am still preferring to have the target name with .a instead of -s as it looks clearer to me. -s is not that obvious when I see it

@stephentoub @ellismg could you have a quick look too?

@jkotas
Copy link
Member

jkotas commented Jan 14, 2016

System.Native.a makes it look like a file name to me, but it is really just a target name. Also, .a is Linux specific (e.g. it is called .lib on Windows).

What about calling the target System.Native-static ?

@stephentoub
Copy link
Member

Test Innerloop Windows_NT Release Build and Test please

@davidsh
Copy link
Contributor

davidsh commented Jan 14, 2016

@tarekgh
Copy link
Member Author

tarekgh commented Jan 15, 2016

I'll sync with the latest to squash other non related commits

tarekgh added a commit that referenced this pull request Jan 15, 2016
Generate System.Native static library to be consumed by corert
@tarekgh tarekgh merged commit 80a884a into dotnet:master Jan 15, 2016
@tarekgh tarekgh deleted the StaticLib branch November 16, 2016 17:58
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Generate System.Native static library to be consumed by corert

Commit migrated from dotnet/corefx@80a884a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants