premake / premake-core

Premake
https://premake.github.io/
BSD 3-Clause "New" or "Revised" License
3.22k stars 620 forks source link

gmake2: issues with perFileFlags #1116

Open tdesveauxPKFX opened 6 years ago

tdesveauxPKFX commented 6 years ago

Small test case for these issues.

    function suite.perfile_apiEmptyValue()
        files { 'a.cpp', 'b.cpp', 'c.cpp' }

        exceptionhandling "Off"

        filter { 'files:b.cpp' }
            exceptionhandling "On"

        prepare()
        test.capture [[
# Per File Configurations
# #############################################

PERFILE_FLAGS_0 = $(ALL_CXXFLAGS) -fno-exceptions
        ]]
    end

In a project with exceptionhandling "Off"and trying to enable exception for one file, gmake2 fail to detect the different configuration. This is due to gcc exceptionhandling On is nil as it is the default.

I managed to make it work locally with: premake.tools.gcc.cxxflags.exceptionhandling.On = "-fexceptions" but, obviously, this is neither a clean nor future proof fix.

Not sure how to fix this, I don't really have time right now to look into it and don't know when I will so if someone want to fix it, please do.

Second issue, https://github.com/premake/premake-core/blob/30ab62418200473203182e3f886bc09de59486bd/modules/gmake2/gmake2_cpp.lua#L572 This does not take into account that for c++ files, toolset.getcxxflags should be called instead of getcflags.

Not really an issue, but still annoying, https://github.com/premake/premake-core/blob/30ab62418200473203182e3f886bc09de59486bd/modules/gmake2/gmake2_cpp.lua#L548 makeVarName being local prevent user from overriding cpp.perFileFlags easily.

These should be easy to fix, but again not really have time right now so just

tdesveauxPKFX commented 6 years ago

I made the changes to fix the second issue and remove the local from makeVarName.

For the first issue, a better local solution would be this:

filter {}
    exceptionhandling "On"
filter { "files: not b.cpp" }
    exceptionhandling "Off"

This produce the result I expected from the code in the unit test posted earlier.

This still feels wrong to me but I do not know what it would entail to "fix" it.