FreeBSD compatibility patches #1705

yurivict · 2021-12-05T20:06:21Z

Description of Change(s)

These patches allow to build USD on FreeBSD.

meshula · 2021-12-05T20:54:06Z

I notice that for the most part, the change involves adding ARCH_OS_FREEBSD to the unix-like blocks. I wonder if it might make sense to define ARCH_OS_LINUX as well as ARCH_OS_FREEBSD, so that only FREEBSD specific cases need to be explicitly addressed?

(Out of scope for what you're trying to do, I have the same question about ARCH_OS_DARWIN. Perhaps ARCH_OS_POSIX, with LINUX, DARWIN, FREEBSD, and MINGW specializations... )

yurivict · 2021-12-05T22:01:20Z

Perhaps ARCH_OS_POSIX, with LINUX, DARWIN, FREEBSD, and MINGW specializations

I agree but this is orthogonal to this patch. The patch adds ifdefs in the same way as the code is now. The change that you propose can be made later for all relevant architectures.

spiffmon · 2021-12-06T00:12:04Z

Agreed - it seems like a nice future simplification to define an ARCH_UNIX_DERIVED with smaller specializations for each of the flavors, but it doesn't seem right to handle BSD as LINUX.

spiffmon · 2021-12-06T00:47:09Z

@yurivict , given that FreeBSD is not represented in our internal build/test infrastructure, and likely will not be anytime soon, and that our azure testing is not covering it either, I just wanted to verify that you have run the full public USD testsuite locally with your changes? Thanks again!

yurivict · 2021-12-06T04:59:50Z

Is it documented how to run the USD testsuite?

spiffmon · 2021-12-06T18:48:28Z

We do not document it, and we should! But all you should need to do is run ctest from the directory from which you ran cmake.

On Sun, Dec 5, 2021 at 9:00 PM ***@***.*** ***@***.***> wrote: Is it documented how to run the USD testsuite? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1705 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPOU2GN72YNCIGTXEOMUH3UPQ7NDANCNFSM5JNEW2VQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- --spiffiPhone

yurivict · 2021-12-06T20:20:52Z

The testsuite looks for test executables in a wrong place:

7: FileNotFoundError: [Errno 2] No such file or directory: '/usr/local/tests/testArchFunction'

The executable path is: ./work/.build/pxr/base/arch/testArchFunction

sunyab · 2021-12-06T22:09:11Z

Hello @yurivict, the build and test infrastructure currently expect that tests will be installed like other build products and run from the installed location. Assuming your cmake install prefix is /usr/local, we would expect /usr/local/tests/testArchFunction and other tests to exist. If it does not, or if your cmake install prefix isn't /usr/local, that indicates that either the build did not fully complete or there is a bug in the build system. In that case, could you please post log files with your cmake and build output so we could have a look?

jilliene · 2021-12-08T23:58:06Z

Filed as internal issue #USD-7057

sunyab · 2021-12-10T22:35:54Z

Hi @yurivict, we are looking at merging these patches in. However, we need a signed CLA from contributors before we are able to merge their changes. Could you please follow the instructions at: https://graphics.pixar.com/usd/release/contributing_to_usd.html

Once we receive the CLA we can move forward. Thank you!

spitzak · 2021-12-17T18:45:53Z

pxr/base/arch/fileSystem.cpp

+            if (kf->kf_path[0])
+            {
+                path.resize(strlen(kf->kf_path) + 1);
+                sprintf(&path[0], "%s", kf->kf_path);


Why not strcpy(path, kf->kf_path)?

spitzak · 2021-12-17T18:49:45Z

It seems like "!ARCH_OS_WINDOWS" could be used for "unix derived". I have also seen "POSIX" used to avoid the Unix trademark.

spiffmon · 2022-01-14T22:55:03Z

Hi @yurivict ! Pinging again here to see if we can get a CLA from you? We'd like to process this PR, but cannot proceed without a CLA.

spiffmon · 2022-04-24T04:47:49Z

Hello, @yurivict - I see from recently filed Issue #1840 that you are still active, here. So I'd like to repeat once more our request for you to file a CLA with us so that we can accept this great contribution. Thank you!

yurivict · 2022-04-24T05:23:46Z

Sorry for the delay. I've sent the signed agreement.

could lead to invalid memory access. (Internal change: 2228809)

(Internal change: 2230317) (Internal change: 2230746)

Fixes PixarAnimationStudios#1901 (Internal change: 2237347)

(Internal change: 2237514)

yurivict · 2022-07-23T00:41:18Z

@spiffmon Could you please merge this PR?

Thanks,
Yuri

spiffmon · 2022-07-23T00:50:06Z

Hi @yurivict ,
Apologies for not updating the PR... @sunyab attempted to merge this PR, and it caused the supported Centos/RHEL build to fail, I believe. He may remember more details, but are you able to test in a Linux environment?

yurivict · 2022-07-23T00:53:18Z

[...] are you able to test in a Linux environment?

No, sorry, I don't have Linux. But the patch should be orthogonal to Linux.

yurivict · 2022-07-23T00:54:57Z

This line:

#if defined(ARCH_OS_LINUX) || defined(ARCH_OS_FREEBSD) && defined(ARCH_COMPILER_GCC)

should be

#if (defined(ARCH_OS_LINUX) || defined(ARCH_OS_FREEBSD)) && defined(ARCH_COMPILER_GCC)

sunyab · 2022-07-23T01:03:37Z

pxr/base/arch/debugger.cpp

@@ -46,6 +46,7 @@
 #include <unistd.h>
 #include <string>
 #endif
+#include <signal.h>


Why is signal.h needed here when csignal is included above?

sunyab · 2022-07-23T01:04:50Z

pxr/base/arch/fileSystem.h

 #include <unistd.h>
-#include <sys/statfs.h>
+//#include <sys/statfs.h>


If this line is unnecessary on Linux and FreeBSD, please remove it instead of commenting it out.

sunyab · 2022-07-23T01:09:31Z

My Apologies for not updating this PR earlier. This PR did fail to build on Linux on Azure, and when I tried it a while back on Linux I got the following error. Aside from that and the other issue you mentioned in your earlier post, I've left two questions/notes on the changes.

base/arch/systemInfo.cpp: In lambda function:
base/arch/systemInfo.cpp:130:44: error: expected ')' before ';' token                                                                                                                                     
                 const ssize_t n = readlink(SELF_PATHNAME, buffer, *size);                                                                                                                                
                                            ^~~~~~~~~~~~~
base/arch/systemInfo.cpp:130:57: error: expected primary-expression before ',' token
                 const ssize_t n = readlink(SELF_PATHNAME, buffer, *size);                                                                                                                                
                                                         ^
base/arch/systemInfo.cpp:132:21: error: expected ')' before ';' token                                                                                                                                     
                     ARCH_WARNING("Unable to read " SELF_PATHNAME " to obtain "
                     ^~~~~~~~~~~~
base/arch/systemInfo.cpp:132:21: error: left operand of comma operator has no effect [-Werror=unused-value]
base/arch/systemInfo.cpp:132:21: error: right operand of comma operator has no effect [-Werror=unused-value]
base/arch/systemInfo.cpp:132:21: error: right operand of comma operator has no effect [-Werror=unused-value]
base/arch/systemInfo.cpp:132:21: error: expected ';' before ')' token                                                                                                                                     
base/arch/systemInfo.cpp:133:52: error: right operand of comma operator has no effect [-Werror=unused-value]
                                  "executable path");                                                                                                                                                     
                                                    ^
base/arch/systemInfo.cpp:140:31: error: expected ')' before ';' token                                                                                                                                     
                     if (lstat(SELF_PATHNAME, &sb) == 0) {
                               ^~~~~~~~~~~~~
base/arch/systemInfo.cpp:140:31: error: expected ')' before ';' token                                                                                                                                     
base/arch/systemInfo.cpp:140:44: error: expected primary-expression before ',' token
                     if (lstat(SELF_PATHNAME, &sb) == 0) {
                                            ^
base/arch/systemInfo.cpp:143:21: error: expected '}' before 'else'
                     else {
                     ^~~~
base/arch/systemInfo.cpp: In function 'std::string ArchGetExecutablePath()':
base/arch/systemInfo.cpp:149:17: error: expected ')' before 'else'
                 else {
                 ^~~~
base/arch/systemInfo.cpp:153:13: error: expected ';' before '}' token                                                                                                                                     
             });                                                                                                                                                                                          
             ^
base/arch/systemInfo.cpp: At global scope:
base/arch/systemInfo.cpp:153:14: error: expected unqualified-id before ')' token
             });                                                                                                                                                                                          
              ^
base/arch/systemInfo.cpp:198:1: error: expected declaration before '}' token
 }
 ^
compiling: arch regex.o
cc1plus: all warnings being treated as errors

yurivict · 2022-07-23T01:35:53Z

It looks like Arch_ComputeNanosecondsPerTick now needs to be implemented for FreeBSD. This will take some time.

yurivict · 2022-07-23T03:03:02Z

@sunyab I've created this new PR #1961 instead.

spiffmon mentioned this pull request Dec 6, 2021

ArchGetFileName is conceptually flawed: reverse lookup of file path by the open file descriptor is generally impossible #1704

Closed

yurivict force-pushed the FreeBSD_patches branch 2 times, most recently from 676c080 to 4942189 Compare December 6, 2021 20:05

spitzak reviewed Dec 17, 2021

View reviewed changes

yurivict force-pushed the FreeBSD_patches branch from 4942189 to ac3969d Compare May 7, 2022 17:01

sunyab added 4 commits May 9, 2022 14:03

Fix race condition during prim destruction that

f4f31bc

could lead to invalid memory access. (Internal change: 2228809)

Update CHANGELOG.md for 22.05a patch release

7d637e4

(Internal change: 2230317) (Internal change: 2230746)

Update download URL for libtiff

7ee1dcd

Fixes PixarAnimationStudios#1901 (Internal change: 2237347)

Update CHANGELOG for 22.05b release

e0e5156

(Internal change: 2237514)

sunyab changed the base branch from release to dev June 18, 2022 00:38

Merge release v22.08

3abc464

sunyab requested changes Jul 23, 2022

View reviewed changes

yurivict added 2 commits July 22, 2022 18:11

FreeBSD compatibility patches.

d987053

Merge branch 'release' into FreeBSD_patches

9cb79af

yurivict force-pushed the FreeBSD_patches branch from 211ddd7 to 9cb79af Compare July 23, 2022 01:15

yurivict closed this Jul 23, 2022

VVD mentioned this pull request Aug 21, 2024

Fails to find oneTBB-2021.3.0 #1650

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FreeBSD compatibility patches #1705

FreeBSD compatibility patches #1705

yurivict commented Dec 5, 2021

meshula commented Dec 5, 2021

yurivict commented Dec 5, 2021

spiffmon commented Dec 6, 2021

spiffmon commented Dec 6, 2021

yurivict commented Dec 6, 2021

spiffmon commented Dec 6, 2021 via email

yurivict commented Dec 6, 2021

sunyab commented Dec 6, 2021

jilliene commented Dec 8, 2021

sunyab commented Dec 10, 2021

spitzak Dec 17, 2021

spitzak commented Dec 17, 2021

spiffmon commented Jan 14, 2022

spiffmon commented Apr 24, 2022

yurivict commented Apr 24, 2022

yurivict commented Jul 23, 2022

spiffmon commented Jul 23, 2022

yurivict commented Jul 23, 2022

yurivict commented Jul 23, 2022

sunyab Jul 23, 2022

sunyab Jul 23, 2022

sunyab commented Jul 23, 2022

yurivict commented Jul 23, 2022

yurivict commented Jul 23, 2022

FreeBSD compatibility patches #1705

FreeBSD compatibility patches #1705

Conversation

yurivict commented Dec 5, 2021

Description of Change(s)

meshula commented Dec 5, 2021

yurivict commented Dec 5, 2021

spiffmon commented Dec 6, 2021

spiffmon commented Dec 6, 2021

yurivict commented Dec 6, 2021

spiffmon commented Dec 6, 2021 via email

yurivict commented Dec 6, 2021

sunyab commented Dec 6, 2021

jilliene commented Dec 8, 2021

sunyab commented Dec 10, 2021

spitzak Dec 17, 2021

Choose a reason for hiding this comment

spitzak commented Dec 17, 2021

spiffmon commented Jan 14, 2022

spiffmon commented Apr 24, 2022

yurivict commented Apr 24, 2022

yurivict commented Jul 23, 2022

spiffmon commented Jul 23, 2022

yurivict commented Jul 23, 2022

yurivict commented Jul 23, 2022

sunyab Jul 23, 2022

Choose a reason for hiding this comment

sunyab Jul 23, 2022

Choose a reason for hiding this comment

sunyab commented Jul 23, 2022

yurivict commented Jul 23, 2022

yurivict commented Jul 23, 2022