premake / premake-xcode

BSD 3-Clause "New" or "Revised" License
9 stars 15 forks source link

Mapping includedirs and sysincludedirs to HEADER_SEARCH_PATHS #26

Closed soundsrc closed 8 years ago

soundsrc commented 8 years ago

Currently, the includedirs option maps to USER_HEADER_SEARCH_PATHS (clang's -iquote option) and sysincludedirs maps to HEADER_SEARCH_PATHS (clang's -I option).

This causes all headers with angled brackets, ie. #include <SomeHeader.h> to be not recognized on the search path when specified with the "includedir" option in premake5.

I've reverted the sysincludedirs changes and remapped includedirs and sysincludedirs to map to HEADER_SEARCH_PATHS ( -I option ) as that seems to be the expected behaviour.

starkos commented 8 years ago

Got it, thanks!

akaStiX commented 8 years ago

HI @soundsrc @starkos I want to bring this up. HEADER_SEARCH_PATHS to my knowledge is used for system headers, i.e. included with both <> and "" and USER_HEADER_SEARCH_PATHS - for application headers, i.e. included with "". So if you have OpenGL.h in you project, you'll end up having it in HEADER_SEARCH_PATHS and if you do this:

#include <OpenGL.h>

you'll end up with your code including your own OpenGL.h header, instead of the one, shipped with XCode and you'll most definitely get compilation errors, because you are missing some of the declarations.

Plus this commit breaks sysincludes, by removing the calls to wrap includes into quotation marks, so this:

sysincludes {
    "../Some Path With Spaces/*.h"
}

will no longer work due to a spaces in the path. changes to tests/test_xcode_project.lua clearly show this. Tests are there for a reason. We have to revert this

soundsrc commented 8 years ago

If the spaces are broken, we should fix it for sure.

I'm not sure if I agree with the old behaviour, but we can debate. I went by the docs: https://github.com/premake/premake-core/wiki/sysincludedirs where sysincludes maps to -isystem and includedirs maps to -I. HEADER_SEARCH_PATHS is the gcc equivalent to -I, which is both system and user headers. And this was the behaviour in premake4. USER_HEADER_SEARCH_PATHS maps to -iquote, which I'm not sure should be used with includedir. Let me know your thoughts.

akaStiX commented 8 years ago

This is a list of paths to folders to be searched by the compiler for included or imported user header files (those headers listed in quotes) when compiling C, Objective-C, C++, or Objective-C++. Paths are delimited by whitespace, so any paths with spaces in them need to be properly quoted. See the description of the "Always Search User Paths" build setting for more details on how this setting is used. If the compiler doesn't support the concept of user headers, then the search paths are prepended to the any existing header search paths defined in "Header Search Paths". [USER_HEADER_SEARCH_PATHS, -iquote]

This is a description of the option in XCode. So this is as you say, but in reality USER_HEADER_SEARCH_PATHS is used to search quoted include paths and HEADER_SEARCH_PATHS used for both. In my message I gave an example where this commit broke my project. And I don't understand, why would you like to merge both includedirs and sysincludedirs. premake5 and premake4 aren't really compatible to my knowledge and we should make things right, not backwards compatible. Plus this commit totally broke sysincludedirs with spaces. So my opinion is that we should revert it and you should use whatever fits your code in your project - i.e. sysincludedirs, if you include everything via <>. Brackets are for system includes - https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html

soundsrc commented 8 years ago

I'm all for not keeping backwards compatible. My other concern with the -iquote mapping is that it breaks compatibility with gmake gcc projects, where includedir mapped to -I, which is both user and system. This was the reason I initially brought this issue up. I'm cool with keeping a private branch, I'll let @starkos make the call on the correct behaviour.

akaStiX commented 8 years ago

Than we have to fix gmake generator to map includedirs to -iquote and sysincludedirs to -I ;)

soundsrc commented 8 years ago

Right now we have -I, -iquote and -isystem. I would love to have clarification in the docs and then we can have consistent behaviour across all platforms.

starkos commented 8 years ago

Open to suggestions here, and in favor of "right" over "compatible".

akaStiX commented 8 years ago

Include syntax has clear separation between user headers aka quoted and system headers aka bracketed. In XCode we have options for USER_HEADER_SEARCH_PATHS and it feels natural for me to map it to includedirs and HEADER_SEARCH_PATHS otherwise should stick to sysinclude. XCode (at least after migration to Clang) doesn't recognize -isystem (as far as i remember) and the "disadvantage" of -isystem is that warning are disabled for them, so that's another point for mapping sysincludes to HEADER_SEARCH_PATHS. I have nothing to say about gmake yet. Maybe it's worth to go the compatibility way i.e. to map includedirs to both -I and -iquote and sysinclude to -isystem

soundsrc commented 8 years ago

Perhaps we need to do something like: includedirs (-I, HEADER_SEARCH_PATHS, most compatible with all systems right now) sysincludedirs (-isystem, in Xcode this would be passed as a custom flag with -isystem since there is no equivalent) userincludedirs (-iquote, USER_HEADER_SEARCH_PATHS, new flag for premake)

soundsrc commented 8 years ago

Oh missed the clang not supporting -isystem. Perhaps, that just has to be a normal -I, HEADER_SEARCH_PATHS.

akaStiX commented 8 years ago

-I is used for both system and user headers. Since we have sysincludedirs, than includedirs feels like user include dirs and should be mapped to USER_HEADER_SEARCH_PATHS

soundsrc commented 8 years ago

if we make includedirs map to only user path, then we would have to make gmake and every other build system behave the same. I'm okay with that as long as the docs be updated to clarify the usage.

akaStiX commented 8 years ago

I am suggesting to map includedirs to both -iquote and -I on gmake, since that would be aligned with documentation and backwards compatible with projects like mine

soundsrc commented 8 years ago

I'm not sure if you can map -iquote and -I together because -I will roll in system headers. The idea of -iquote is user headers but not system headers. We would have ensure MSVC and other build systems would behave the same. We should definitely be careful here about the exact behaviour.

I personally would like to have three include flags where this is clearly defined. One is both user and system (-I), one is user path only (-iquote) and one is system path only (-isystem).

Whatever system is decided, it should be consistent across all platforms and clearly documented.

soundsrc commented 8 years ago

I've sent a request for the revert. I think fixing the blocking issues first and then we can debate on the correct behaviour after analysing it more.