xrootd: fix missing byteswap.h error on MacOS build with gcc

[barracuda156]

This PR fixes a missing header error on MacOS when building with GCC.
Confirmed to fix the build on 10.6.8 with gcc12.

Clang builds on latest MacOS versions are all good too: macports/macports-ports#15807 (checks pass).

Code borrowed from: https://gerrit.pixelexperience.org/plugins/gitiles/external_libtextclassifier/+/b23e2125be90bbf6124e9cd5684fc93026c5ec4d/util/base/endian.h

[barracuda156]

Build bot error seems unrelated:

[ 94%] Built target xrdpfc_print
[2849](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2850)
[ 95%] Building CXX object src/XrdCl/CMakeFiles/xrdfs.dir/XrdClFS.cc.o
[2850](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2851)
/Users/runner/work/xrootd/xrootd/src/XrdPfc/XrdPfcFile.cc:654:41: error: use of undeclared identifier 'EBADFD'
[2851](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2852)
      return m_in_shutdown ? -ENOENT : -EBADFD;
[2852](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2853)
                                        ^
[2853](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2854)
/Users/runner/work/xrootd/xrootd/src/XrdPfc/XrdPfcFile.cc:683:41: error: use of undeclared identifier 'EBADFD'
[2854](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2855)
      return m_in_shutdown ? -ENOENT : -EBADFD;
[2855](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2856)
                                        ^
[2856](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2857)
2 errors generated.
[2857](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2858)
gmake[2]: *** [src/CMakeFiles/XrdPfc-5.dir/build.make:132: src/CMakeFiles/XrdPfc-5.dir/XrdPfc/XrdPfcFile.cc.o] Error 1
[2858](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2859)
gmake[1]: *** [CMakeFiles/Makefile2:1945: src/CMakeFiles/XrdPfc-5.dir/all] Error 2
[2859](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2860)
[ 95%] Building CXX object src/XrdCl/CMakeFiles/xrdfs.dir/XrdClFSExecutor.cc.o
[2860](https://github.com/xrootd/xrootd/runs/7979714524?check_suite_focus=true#step:5:2861)
gmake[1]: *** Waiting for unfinished jobs....

Our builds in Macports on Intel systems are fine.

[adriansev]

@barracuda156 I think that you would need to move your definition to XrdSys/XrdSysPlatform.hh
where __bswap_64 is aliased to htonll (and seems anyway to be the location for platform oriented fixes). Also, seems that direct usage of bswap in XrdOssCsiTagstoreFile is not really correct as in the interest of platforms support, the aliases from XrdSysPlatform.hh should be used.
@simonmichal can give an authoritative answer on this ..

[barracuda156]

@barracuda156 I think that you would need to move your definition to XrdSys/XrdSysPlatform.hh
where __bswap_64 is aliased to htonll (and seems anyway to be the location for platform oriented fixes). Also, seems that direct usage of bswap in XrdOssCsiTagstoreFile is not really correct as in the interest of platforms support, the aliases from XrdSysPlatform.hh should be used.
@simonmichal can give an authoritative answer on this ..

The module in question is built only when GCC is used, Clang builds skip it, and since 10.7 Clang is the default compiler in Xcode and Macports – that is why no one noticed the error. So while the header in question is missing on MacOS generally, it is invoked only in one file, which I patched.

But if you advise moving the definitions to XrdSysPlatform.hh, let’s do it.

[adriansev]

@barracuda156 well, let's see what @simonmichal has to say ..
but my advice was because i seen that __bswap_64 have aliases in XrdSysPlatform.hh, that in turn are used widely in the code..

[simonmichal]

@barracuda156 : thanks for the PR!

I don't have any strong opinion about moving it to XrdSysPlatform.hh. We also use bswap directly in XrdOucCRC32C.cc, though I think this is unchanged code from Mark Adler. Let's see if @abh3 or @smithdh (the author of this plug-in) have a clear advice.

[cjones051073]

Just to note I am seeing this same build failure on macOS12 with Xcode clang, so it is not just affecting gcc builds. See attached build log.

main.log

The previous 5.4.x version built fine, so this appears to be new with 5.5.0.

[cjones051073]

Just to note I am seeing this same build failure on macOS12 with Xcode clang, so it is not just affecting gcc builds. See attached build log.

main.log

The previous 5.4.x version built fine, so this appears to be new with 5.5.0.

erm, apologies, I should have checked the log better....

it seems the above is picking macports gcc to build. Need to investigate why that is...

[smithdh]

Thanks to barracuda156 for the fix. It seems XrdOssCsiTagstoreFile.hh(cc) is almost the only place in the xrootd code base which uses this functionality. (The exception being XrdOucCRC32C.cc, but this is almost Mark Adler's released code plain, so we may not want to adapt it with local changes to use XrdSysPlatform specifics).

As it's something unique to TagstoreFile.hh I don't think it needs to be moved from TagstoreFile.hh. In the moment in it is needed elsewhere it could be moved to XrdSysPlatform.hh for a common location; although perhaps if we've had a history of multiple definitions of utility functions cropping up over time in the project, that could be a reason to forestall duplication by adding it to XrdSysPlatform now (I'll let Andy comment if he does feel that's a risk and something he'd like to avoid).

The htonll mentioned in XrdSysPlatform.hh is not bswap in terms of htonll, but the other way around. Generally htonll need not be not the same as bswap_64, but on many platforms it is. The way that bswap_64/32 is used in TagstoreFile.hh/cc is that it expects a function that swaps in any case, it can't be substituted with htonll as written.

[barracuda156]

it seems the above is picking macports gcc to build

@cjones051073 UPD: Ah, you refer to the attached log from another build. Normally Macports chooses Clangs for all Intel systems >10.6, though gcc can be chosen manually via configure.compiler=macports-gcc-12, for example.

Even though Clang is a default choice on Apple usually, GCC case should work too. For PPC it is also the only choice, since Clang is broken there.

[xrootd-dev]

Indeed, we would not want to duplicate code and the particular definition for the PPC Mac version (isn't that platform no longer supported?) would rightly be placed in SysPlatform.hh. As for XrdOucCRC32C.cc, isn't it the case that the code already takes care of this problem? Andy

…

On Mon, 5 Sep 2022, David Smith wrote: Thanks to barracuda156 for the fix. It seems XrdOssCsiTagstoreFile.hh(cc) is almost the only place in the xrootd code base which uses this functionality. (The exception being XrdOucCRC32C.cc, but this is almost Mark Adler's released code plain, so we may not want to adapt it with local changes to use XrdSysPlatform specifics). As it's something unique to TagstoreFile.hh I don't think it needs to be moved from TagstoreFile.hh. In the moment in it is needed elsewhere it could be moved to XrdSysPlatform.hh for a common location; although perhaps if we've had a history of multiple definitions of utility functions cropping up over time in the project, that could be a reason to forestall duplication by adding it to XrdSysPlatform now (I'll let Andy comment if he does feel that's a risk and something he'd like to avoid). The htonll mentioned in XrdSysPlatform.hh is not bswap in terms of htonll, but the other way around. Generally htonll need not be not the same as bswap_64, but on many platforms it is. The way that bswap_64/32 is used in TagstoreFile.hh/cc is that it expects a function that swaps in any case, it can't be substituted with htonll as written. -- Reply to this email directly or view it on GitHub: #1766 (comment) You are receiving this because you are subscribed to this thread. Message ID: ***@***.***> ######################################################################## Use REPLY-ALL to reply to list To unsubscribe from the XROOTD-DEV list, click the following link: https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1

[barracuda156]

Indeed, we would not want to duplicate code and the particular definition for the PPC Mac version (isn't that platform no longer supported?) would rightly be placed in SysPlatform.hh. As for XrdOucCRC32C.cc, isn't it the case that the code already takes care of this problem? Andy
…

This is not specific to MacOS X PPC (we do support it in Macports though), but to a combination of MacOS X and GCC. What fails is GCC-only module:

xrootd/src/CMakeLists.txt

Line 63 in 7cc1184

include( XrdOssCsi )

So it gonna fail on every MacOS likewise, since that header is not there.

[smithdh]

Would it be possible to rebase this PR to the current xrootd master; doing so should clear the failing build check (which happens to be the one for macos)?

[smithdh]

Hi @barracuda156; thank you for the pull request. I have merged it, but due to an oversight the commit lost your author name (although the log message includes the reference to your repository). I'm sorry about that.

[barracuda156]

Hi @barracuda156; thank you for the pull request. I have merged it, but due to an oversight the commit lost your author name (although the log message includes the reference to your repository). I'm sorry about that.

@smithdh No issues, thank you for merging!