Fix FreeBSD build failures. Update tests for FreeBSD#92
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Thefrank commented Mar 3, 2023
@microsoft-github-policy-service agree |
Thefrank commented Aug 5, 2023
Not sure who I should get code review from / who has write access so I am guessing based on recent PR merges PTAL @adityapatwardhan |
Thefrank commented Aug 20, 2023
If there is nothing else, then this is ready to merge |
shanselman commented Oct 11, 2023
@SteveL-MSFT this is the last one I think that's needed for dotnet/runtime#14537 upstream |
src/libpsl-native/CMakeLists.txt Outdated
| 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!
| 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.
| pid_tGetPPid(pid_t pid) | ||
| { | ||
| #if defined (__APPLE__) && defined(__MACH__) || defined(__FreeBSD__) |
Thefrank commented Jul 2, 2024
What else is needed to get this merged? Any feedback would be great! |
adityapatwardhan commented Sep 4, 2024
/azp run |
| Azure Pipelines successfully started running 1 pipeline(s). |
adityapatwardhan commented Sep 4, 2024
@Thefrank thank you for your contribution! |
4 participants
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