Fix FreeBSD build failures. Update tests for FreeBSD #92
Conversation
|
@microsoft-github-policy-service agree |
|
Not sure who I should get code review from / who has write access so I am guessing based on recent PR merges PTAL @adityapatwardhan |
|
If there is nothing else, then this is ready to merge |
|
@SteveL-MSFT this is the last one I think that's needed for dotnet/runtime#14537 upstream |
src/libpsl-native/CMakeLists.txt
Outdated
| @@ -9,6 +9,8 @@ if (${CMAKE_SYSTEM_NAME} MATCHES "Linux") | |||
| set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-z,relro,-z,now") | |||
| elseif (${CMAKE_SYSTEM_NAME} MATCHES "Darwin") | |||
| set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl") | |||
| elseif (${CMAKE_SYSTEM_NAME} MATCHES "FreeBSD") | |||
| set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl") | |||
There was a problem hiding this comment.
It seems strange to pass -Wl with no arg. Of course the oddity was just duplicated from the Darwin case (where it exists in the first public version of this file).
It seems to me we ought to just mimic the Linux behaviour here, e.g. make the existing case above if (${CMAKE_SYSTEM_NAME} MATCHES "Linux" OR ${CMAKE_SYSTEM_NAME} MATCHES "FreeBSD")
There was a problem hiding this comment.
I found it unusual too, but opted to include it as I assumed if it was important enough for "Darwin" to have it then so should "FreeBSD". I will go ahead and add "FreeBSD" to the "Linux" case as suggested. Thanks!
| @@ -17,6 +21,9 @@ pid_t GetCurrentThreadId() | |||
| uint64_t tid64; | |||
| pthread_threadid_np(NULL, &tid64); | |||
| tid = (pid_t)tid64; | |||
| #elif defined(__FreeBSD__) | |||
| tid = pthread_getthreadid_np(); | |||
| return (int)tid; | |||
There was a problem hiding this comment.
This cast is superfluous - this function returns pid_t, which is the type of tid. Probably want in the line above tid = (pid_t)pthread_getthreadid_np();, or just avoid explicit casts altogether.
There was a problem hiding this comment.
This one...I tried my best here to follow what is done under dotNET for pthread_getthreadid_np() but found it inconsistent. I can make changes both to this PR and open PRs in runtime to correct the usage it in order to avoid any information loss due to casting.
| @@ -31,7 +31,7 @@ | |||
| //! | |||
| pid_t GetPPid(pid_t pid) | |||
| { | |||
|
|
|||
| #if defined (__APPLE__) && defined(__MACH__) || defined(__FreeBSD__) | |||
|
What else is needed to get this merged? Any feedback would be great! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@Thefrank thank you for your contribution! |
Fix FreeBSD build failures. Update tests for FreeBSD
Changes
statThere are still two test failures related to GetDeviceID. FreeBSD returns a nonsensically high number for
/usr/bin/stat -f %d /Ubuntu 20.04:
FreeBSD 13.1:
Reasoning
Current FreeBSD builds fail. This PR resolves it and adds supporting tests