Skip to content

Do not expose AODToHepMC.h to ROOT#12248

Merged
ktf merged 1 commit intoAliceO2Group:devfrom
ktf:pr12248
Nov 14, 2023
Merged

Do not expose AODToHepMC.h to ROOT#12248
ktf merged 1 commit intoAliceO2Group:devfrom
ktf:pr12248

Conversation

@ktf
Copy link
Copy Markdown
Member

@ktf ktf commented Nov 14, 2023

Do not expose AODToHepMC.h to ROOT

This is needed to fix macOS builds. Apparently ROOT injects
some arrow incompatible system headers in the chain.

@ktf
Copy link
Copy Markdown
Member Author

ktf commented Nov 14, 2023

Fixes issue introduced in #12038.

@cholmcc
Copy link
Copy Markdown
Contributor

cholmcc commented Nov 14, 2023

The problem is not that

ROOT injects some arrow incompatible system headers in the chain

but rather that Apple - in its infinite wisdom - has decided to change a BSD header (sys/vnode.h) to define the preprocessor macro PREALLOCATE. The same name is used as an identifier in arrow/compute/kernel.h which causes the clash. Given that it is unlikely that Apple will change its broken header, a fix could be to change the arrow header as in this (a bug report has been filed with upstream too).

Of course, a way to hack it is not let ROOT see AODToHepMC.h as done in this MR. The culprit seems to be the inclusion of Framework/AnalysisDataModel.h which indicates that we would not have dictionaries for the types declared there. I don't know if that is such a good idea.

Yours,
Christian

@ktf
Copy link
Copy Markdown
Member Author

ktf commented Nov 14, 2023

sys/vnode.h is a private platform header, so Apple can basically do what they want with it.

Also, I am not convinced the problem is not root related. Compiling with:

c++ -std=c++17 -I../sw/osx_arm64/ms_gsl/latest/include -I../sw/osx_arm64/fmt/latest/include/ -I../sw/osx_arm64/FairLogger/latest/include -I../sw/osx_arm64/FairMQ/latest/include/ -I../sw/osx_arm64/boost/latest/include/ -I../sw/osx_arm64/ROOT/latest/include -I../sw/osx_arm64/arrow/latest/include/ -I../sw/osx_arm64/O2/latest-12248-o2/include -I/opt/homebrew/include  -E Framework/Core/include/Framework/AnalysisManagers.h -I../sw/osx_arm64/libuv/latest/include/

works fine on the same machine and there is no "vnode.h" included. I am afraid you have little chances to have the arrow header modified, since they will simply say that vnode.h is not included by kernel.h.

Finally, there should not be any need to include AnalysisManagers.h nor any arrow related header in a ROOT dictionary, so I think this is in any case the best solution.

This is needed to fix macOS builds. Apparently ROOT injects
some arrow incompatible system headers in the chain.
@ktf ktf merged commit 3bae263 into AliceO2Group:dev Nov 14, 2023
@ktf ktf deleted the pr12248 branch November 14, 2023 18:04
@cholmcc
Copy link
Copy Markdown
Contributor

cholmcc commented Nov 15, 2023

Hi Guilio,

Yes Apple can in some respect do what they want, but then again not really if they want to claim OpenSource and BSD. But as I said, fat chance getting them to change anything.

Sure, it is ROOT that somewhere (probably very deep) pulls in sys/vnode.h (it is a file-system access header), and the issue could probably be fixed there too - e.g., by doing something a la

#include <vnode.h>
#if defined(__APPLE__) && defined(PREALLOCATE)
# undef PREALLOCATE
#endif           

The header vnode.h could also be pulled in by some C++ header, e.g., #include <filesystem> which then causes the conflict.

We do have dictionaries for some arrow tables, I believe. For example McTracks and similar, and it may make sense to also have them for other tables - e.g., AOD tables.

Don't get me wrong, I think the hack you did is fine, but I do think my comment stands in the sense that it points to a deeper issue that may become a problem.

Yours,
Christian

@ktf
Copy link
Copy Markdown
Member Author

ktf commented Nov 16, 2023

Yes Apple can in some respect do what they want, but then again not really if they want to claim OpenSource and BSD. But as I said, fat chance getting them to change anything.

I am not sure what OpenSource and BSD have to do with it. The header in question can be found at:

https://opensource.apple.com/source/xnu/xnu-201.5/bsd/sys/vnode.h.auto.html

which is both OpenSource and BSD licensed. If you mean they do not adhere to some standard, they only claim to comply to POSIX (i.e. "UNIX"), which does not mention sys/vnode.h at all. That said, as you can see the conflicting code itself is protected by #ifdef KERNEL, and as far as I can tell, nothing sets it in arrow nor in our code. IMHO, it's something fishy in ROOT, but I have no time right now to investigate it further.

The header vnode.h could also be pulled in by some C++ header, e.g., #include which then causes the conflict.

No, it does not. You can check for yourself with the c++ command I have above. It will dump the whole expanded sourcefile for that header, and you'll see that vnode.h is not even included (nor KERNEL is defined).

@cholmcc
Copy link
Copy Markdown
Contributor

cholmcc commented Nov 16, 2023

I am not sure what OpenSource

Look at the URL you posted- it literally starts with opensource :-)

and BSD

Again, the URL you posted says what BSD has to do with it: bsd/sys/vnode.h - if you need more, check the XNU GitHub repo, where it states

XNU is a hybrid kernel combining the Mach kernel developed at Carnegie Mellon University with components from FreeBSD ...

Clearly, the header in question comes from the FreeBSD source code.

... is protected by #ifdef KERNEL ...

That condition starts on line 70 and ends on line 79. The problematic define isn't till line 135. It probably should have been protected given that it seems to be used only once and by an internal kernel call - makes you wonder what else the guys and gals at Apple have messed up.

... they only claim to comply to POSIX (i.e. "UNIX") ...

BSD is not POSIX compliant. In their own words it is "mostly POSIX compliant".

XNU is, by extension, not POSIX compliant either - heck, the acronym X is Not UNIX would suggest as much :-)

Note, I didn't mention POSIX, since BSD is not POSIX compliant. All I said is that other BSDs does not seem to have made the same stupid mistake that Apple has, and therefore the test #if defined(__APPLE__) should suffice.

No, it does not

That's a relatively bold statement from a single header. What I said was that some C++ header, perhaps <filesystem>, may pull in sys/vnode.h on MacOSX. The quoted command-line probably won't show that, since it doesn't include <filesystem> directly or indirectly (at least not on GNU/Linux - all I can test on, since I don't use overpriced machines) - and I think that header is a prime candidate to include <sys/vnode.h> possibly through <cstdio>.

I couldn't find any place in the ROOT source code where the header <sys/vnode.h> is included - at least not directly, it could come from the plethora of other 3rd-party libraries that ROOT and O2 links against - perhaps some Boost library. The way to know would be to on a MacOSX machine revert the change of this MR, and then do a preprocessing of the commandline that fails - that could be interesting.

In any case, it seems that arrow may want to fix the problem from their side by replacing PREALLOCATE with kPreallocate. Apple is still the culprit, but at least it won't be a problem for us (when we actually update our arrow fork) and other people as well.

Be it as it may, we have a hack, and that's fine.

Yours,
Christian

@cholmcc
Copy link
Copy Markdown
Contributor

cholmcc commented Nov 29, 2023

The problem has been fixes in upstream Apache Arrow - so now we just need to sync our patched fork 😄

@ktf
Copy link
Copy Markdown
Member Author

ktf commented Nov 30, 2023

Clearly, the header in question comes from the FreeBSD source code.

BSD != FreeBSD, BSD is just a license, and has nothing to do with the actual code you license with it. They are indeed FreeBSD based, and that does not bind them to any particular non-POSIX API, as per BSD license. Apple is POSIX compliant and certified as such. You do not need to have a UNIX / BSD derived kernel to be so. Even Windows 2000 was certified POSIX compliant, and it's clearly not a UNIX kernel.

Setting aside that. I am glad that Arrow adapted their side of the code. That still does not explain how come the code only breaks when compiled with cling, but not when compiled standalone, so I still believe the issue is cling here. Anyway, does not matter, we will update as time permits.

@cholmcc
Copy link
Copy Markdown
Contributor

cholmcc commented Nov 30, 2023

BSD != FreeBSD,

To quote Wikipedia:

[BSD] is a discontinued operating system .... The term "BSD" commonly refers to its open-source descendants,
including FreeBSD, OpenBSD, NetBSD, and DragonFly BSD.

so I guess a more accurate description of BSD would be to say that it is a family of operating systems.

BSD licenses are, again quoting Wikipedia

BSD licenses are a family of permissive free software licenses ...

so not a single particular license but a whole family of licenses. Thus, I think

... and has nothing to do with the actual code you license with it.

is wrong.

The BSD part of the MacOSX kernel (XNU) is based on FreeBSD licensed under the so-called 2 clause BSD license, which is considered OpenSource.

For the header in question <sys/node.h> it seems that Apple has more or less copied that from FreeBSD and then added faulty defines. They are of course allowed to do so under the license of FreeBSD, but it doesn't change that the additions are pretty bad.

I do not believe BSD is POSIX compliant, nor that POSIX has anything to say about the header <sys/vnode.h>. XNU is, as you say POSIX certified, but that's largely due to the extra stuff Apple as put on top of and next to the FreeBSD stuff.

You do not need to have a UNIX / BSD derived kernel to be so.

I know. POSIX is a specification of interface - broadly understood, not implementation so much.

Even Windows 2000 was certified POSIX compliant, and it's clearly not a UNIX kernel.

According to Wikipedia, Windows NT could be made mostly POSIX compliant by a dedicated subsystem, but it doesn't look like it was certified. I believe Windows NT (or WNT <- VMS, like HAL -> IBM) was largely based on VMS. Heck, it seems even DOS could have some claim to POSIX 😄

Of course, as Richard Stallmann likes to point out, an operating system is more than a kernel.

I am glad that Arrow adapted their side of the code.

yes, good to see that there are projects that will fix things, even if it wasn't technically their problem.

Yours,

Christian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants