Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for vendored includes on linux. #2021

Closed
StanleySweet opened this issue Jan 15, 2023 · 16 comments · Fixed by #2028
Closed

Add support for vendored includes on linux. #2021

StanleySweet opened this issue Jan 15, 2023 · 16 comments · Fixed by #2028

Comments

@StanleySweet
Copy link

StanleySweet commented Jan 15, 2023

What problem will this solve?
There is a problem with some (most) linux distro-provided mozjs-/jspackage (or whatever spidermonkey (any version) is packaged as on that system), in that it contains a copy of icu headers that don't match the version of icu installed on the system. This mismatch causes a linking issue in 0 A.D.'s Empire Ascendant build process, preventing us to use system provided libraries.

What might be a solution?
This revision contains a possible solution, by telling gcc (and clang, as it follows enough of the same code path within premake) to look through the mozjs-* includes directory after the system standard directories. This means that gcc/clang will look for icu in the system standard location (and find it) before searching the mozjs-* location.

What other alternatives have you already considered?
Keeping a bundled copy of the library forever.

@StanleySweet StanleySweet changed the title Add support for vendored in includes on linux. Add support for vendored includes on linux. Jan 15, 2023
@nickclark2016
Copy link
Member

It looks like the pkgconfig object in that review above is something provided by Wildfire Games, not core Premake. Currently, would the externalincludes API work for your needs? In GCC and Clang for Makefiles, it'll add -isystem flags. GCC also has the -idirafter include flag, would this be more along the lines of what you need?

@seragh
Copy link

seragh commented Jan 24, 2023

Yes, this is a basically a request for support of -idirafter in core Premake.

@nickclark2016
Copy link
Member

Would be fairly straight forward to implement. What behavior would you expect for XCode and Visual Studio exporters? Would it just append to external/system includes?

@seragh
Copy link

seragh commented Jan 24, 2023

Well, depends on premake policy what to do with XCode and VS exporters or whether this is wontfix. :)

As far as Wildfire Games is concerned the problem is currently limited to Linux distributions installing the shipped ICU headers alongside their mozjs package, limiting this effectively to gcc and clang.

@nickclark2016
Copy link
Member

I'm not necessarily saying it's a won't fix, as this is a pretty straight forward thing to add. I'm just trying to determine how to correctly map the flag to XCode and Visual Studio exporters, or if it's ignored by those exporters.

@seragh
Copy link

seragh commented Jan 24, 2023

Me thinks there is no correct answers, so erroring out if not supported might also be an option?

@s0600204
Copy link

As far as I can make out, the XCode exporter uses either the gcc or clang toolset, whilst the VS exporter uses either the msc or clang toolset. For all three of these toolsets there are suggested modifications at the Revision linked in the Issue description above. Modifying the actual exporters does not appear to be necessary (though you'd be more familiar with that than we are).

Would it be easier if I filter out the modifications to premake core that are proposed in the above Revision and upload it as a pull request here?

@nickclark2016
Copy link
Member

I'm fine with those changes above, but the Visual Studio and XCode exporters are exporting files for the IDEs, whereas gcc, clang, and msc toolsets are more geared towards output needed for CLI and Makefiles. Since I don't believe VS and XCode have an equivalent to idirafter in their project structured, I'd be inclined to append those to the ExternalIncludePath in Visual Studio and the SYSTEM_HEADER_SEARCH_PATHS in Xcode.

@starkos @samsinsane Y'all have any thoughts on that? (Relevant GCC Documentation: https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html)

@s0600204
Copy link

Fair enough. I look forward to seeing what you come up with.

@nickclark2016
Copy link
Member

I think I've left enough time for discussion. I'll try to get started on an implementation soon.

@nickclark2016
Copy link
Member

@s0600204 I've got the review out. Hope to merge it in soon. Feel free to pull down the branch and test it out.

@samsinsane
Copy link
Member

Sorry it's taken me so long to respond to this, haven't had much time for Premake this year. I'm not sure I fully understand the problem? So, I get that you're having issues including header.h because it's getting the wrong one, but how would you include both if you needed to? To me, it seems like the solution to including both, is also the solution to getting the right one? Is mozjs just completely misconfigured in regards to these .pc files you seem to be using to get the include directories, or am I just not understanding how this system works?

@s0600204
Copy link

This has nothing to do with pkgconfig (the ".pc files").

Our problem is that the dependency spidermonkey (by default) bundles the headers for its dependency icu in with its own headers but does not build and vendor any compatible icu library files (libicui18n.so, libicudata.so, ...). (spidermonkey doesn't seem to need them itself.)

Our code, which also depends on icu, needs to include the headers for icu; and as (by default) commandline-specified header locations are searched prior to system standard header locations, the icu headers bundled with spidermonkey are found first and subsequently used.

When we then get to the linking stage: because our code needs to link against built icu libraries, a search is made for them. spidermonkey doesn't bundle them, so the icu libraries installed in the system standard location are found and used.

There is no guarantee that the icu libraries found in the system standard locations are compatible with the icu headers bundled in with spidermonkey - in fact they rarely are - and thus the linking step fails.

One solution would be to tell spidermonkey not to bundle icu headers in with its own - and that is indeed what we do with the copy of spidermonkey that we vendor.

However, many linux distro repository maintainers (understandably) would like to use the version of spidermonkey that they provide through their package repositories, and whilst a few do tell the spidermonkey build process not to vendor icu, most don't.

So an alternate solution is to shift the spidermonkey header location in the header search path order, placing it after the system headers. But to do that (cleanly) requires support from premake.

@samsinsane
Copy link
Member

@s0600204 Yes, I understood the issue was the duplicate files. I'm just not sure I understand how pkgconfig isn't the issue? I don't use it, so you may need to explain what I'm missing, sorry. When I use SDL2 on Linux, I can #include <SDL2/SDL.h> and I will get the SDL2 header installed on the platform, even if another package had an SDL2/SDL.h I would never get that file, because I don't add any of the subfolders of /usr/include to my include search path - isn't that what pkgconfig is doing?

I apologise if it seems like I'm being difficult here, that is very much not my intention. If that's "just how it is" that's fine, but it seems like there's something I'm otherwise missing?

@seragh
Copy link

seragh commented Apr 16, 2023

@samsinsane,

I think the bit you are missing is that pkgconfig won't return a path for libs that are in the default / system include directory, so you can't prepend the system ICU to mozjs which is not in the default / system include directory.

@samsinsane
Copy link
Member

@seragh I reread through the patch link above, and the linked issue in that after posting before. I think I understand the issue, there was a comment in there saying:

The reason why mozjs52 needs a specific path (e.g. /usr/include/mozjs-52) is because it is possible to have multiple mozjs* versions installed at the same time and thus there is a need to point to the correct one.

This was the bit I wasn't considering. In my head, I couldn't follow why you would do #include <header.h> over #include <mozjs-52/header.h> but obviously what I was missing is that 52 needs to change and the only real option to do that is through include directories.

@s0600204 @seragh Sorry I wasn't following along, it seems painfully obvious now 🤦

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

Successfully merging a pull request may close this issue.

5 participants