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

Swap order of applying project specific config #1125

Merged

Conversation

TurkeyMan
Copy link
Contributor

the config needs to be resolved based on the isolated config|platform pair

The issue this is solving is that anything that changes "system" as part of the
config|platform pair would not be applied to resolve of per file config due to
prj.system being a table that contains the current os system and when the order
of application was in the original order the project pair settings would be
overriden by the generic prj settings which are set as defaults in other locations

the config needs to be resolved based on the isolated config|platform pair

The issue this is solving is that anything that changes "system" as part of the
config|platform pair would not be applied to resolve of per file config due to
prj.system being a table that contains the current os system and when the order
of application was in the original order the project pair settings would be
overriden by the generic prj settings which are set as defaults in other locations
@samsinsane
Copy link
Member

I'm not quite following what you're saying, are you able to provide a small snippet that triggers issue?

@TurkeyMan
Copy link
Contributor Author

@bliz-jurecka care to elaborate the problem case? :)

@bliz-jurecka
Copy link
Contributor

bliz-jurecka commented Jul 10, 2018

Hey guys,

in context.mergeFilters the terms are always overridden (except tags) for anything that is provided in both the context and source.

"system" is added to all project configs in the addCommonContextFilters function.

so in the case of a config that wants to change system then use that to add specific settings the value will always be the os.target() value ( use of --os does override this behavior)

a code example would be as follows.

workspace "premaketest"
    configurations { "Debug", "Release" }

    if string.match(_ACTION,'vs*') then
        platforms { 'x86', 'x86_64', 'ARM' }
    end

    filter { 'platforms:ARM' }
        system 'android'
        toolset 'clang'
        toolchainversion(iif(_ACTION >= "vs2015", '3.8', '3.6'))

    filter { 'platforms:x86_64' }
        system 'windows'
    
    filter { 'platforms:x86' }
        system 'windows'
        
    filter {}

    location ( _OPTIONS["to"] )
    cppdialect 'C++11'
    cdialect 'C11'

    exceptionhandling "Off"
    floatingpoint "Fast"
    rtti "Off"
    vectorextensions "SSE2"
    symbols "On"
    warnings "Extra"

    flags { "FatalWarnings", "MultiProcessorCompile" }

    filter { "configurations:Debug" }
        defines { "_DEBUG" }
        optimize "Off"

    filter { "configurations:Release" }
        defines { "NDEBUG" }
        optimize "On"

dofile "src/premake5.lua"
project 'testApp'
    kind 'StaticLib'

    includedirs {
        '../include',
        '.'
    }

    files {
        '../include/**.h',
        '../include/**.inl',
        '**.h',
        '**.c',
        '**.cpp',
        '**.mm',
        '**.m'
    }

    filter { 'system:windows' }
        systemversion "10.0.15063.0"

    filter { 'system:windows', 'files:not win32*' }
        flags 'ExcludeFromBuild'

    filter { 'system:android', 'files:not android**' }
        flags 'ExcludeFromBuild'

In this example the workspace defines the system to use for a specific platform and the project file changes file config properties based on the system that is in use.

when this change is not present the project level version of "system" is going to override the platform level setting. This also enables multi platform solution files to be created as the "system" value can be used to switch out what is compiled when and with what specific configuration.

Let me know if there are still questions.

have a nice day.

@samsinsane
Copy link
Member

@bliz-jurecka thanks for that explanation, I think I've encountered this before and figured it was intended behaviour. I'm happy for this to go through, but I think that @starkos should probably weigh in before it gets merged just in case there's a known problem with swapping these around.

Copy link
Member

@starkos starkos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any obvious issue with this, seems like a reasonable solution.

@TurkeyMan
Copy link
Contributor Author

FWIW, I felt the same way with this patch, and Jason went through it in depth with me. It seems right, or... at least, superior.

@TurkeyMan TurkeyMan merged commit a713ecc into premake:master Jul 10, 2018
@TurkeyMan TurkeyMan deleted the project_specific_config_order_switch branch July 17, 2018 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants