.rc files exclusion for non-windows systems

[yuyoyuppe]

Hi all!

My project currently has single .sln for multiple platforms(Windows and Xbox). I need *.rc files for Windows build, but must exclude it for Xbox one. Typically we would use flags for that:

filter { "files:**.rc", "platforms:Xbox" }
  flags{"ExcludeFromBuild"}

But it doesn't work, because in modules\vstudio\vs2010_vcxproj.lua we're setting checkFunc for resources like that:

            m.emitFiles(prj, group, "ResourceCompile", nil, fileCfgFunc, function(cfg)
                return cfg.system == p.WINDOWS
            end)

...which prevents m.excludedFromBuild from being invoked, since I'm relying on .system for my Xbox module.

I've fixed the issue like that:

diff --git a/modules/vstudio/tests/vc2010/test_files.lua b/modules/vstudio/tests/vc2010/test_files.lua
index 51019fad..89ab5164 100644
--- a/modules/vstudio/tests/vc2010/test_files.lua
+++ b/modules/vstudio/tests/vc2010/test_files.lua
@@ -278,7 +278,9 @@
        prepare()
        test.capture [[
 <ItemGroup>
-   <ResourceCompile Include="hello.rc" />
+   <ResourceCompile Include="hello.rc">
+       <ExcludedFromBuild>true</ExcludedFromBuild>
+   </ResourceCompile>
 </ItemGroup>
        ]]
    end
@@ -295,6 +297,7 @@
 <ItemGroup>
    <CustomBuild Include="hello.cg">
        <FileType>Document</FileType>
+       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">true</ExcludedFromBuild>
        <Command Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">cgc $(InputFile)</Command>
        <Outputs Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(InputName).obj</Outputs>
    </CustomBuild>
diff --git a/modules/vstudio/vs2010_vcxproj.lua b/modules/vstudio/vs2010_vcxproj.lua
index 4ba96c6d..cbcab19a 100644
--- a/modules/vstudio/vs2010_vcxproj.lua
+++ b/modules/vstudio/vs2010_vcxproj.lua
@@ -1167,8 +1167,11 @@
                    m.conditionalElements = {}
                    for cfg in project.eachconfig(prj) do
                        local fcfg = fileconfig.getconfig(file, cfg)
+                        local configPair = m.configPair(cfg)
                        if not checkFunc or checkFunc(cfg, fcfg) then
-                           p.callArray(fileCfgFunc, fcfg, m.configPair(cfg))
+                           p.callArray(fileCfgFunc, fcfg, configPair)
+                        else
+                            m.element("ExcludedFromBuild", configPair, "true")
                        end
                    end
                    if #m.conditionalElements > 0 then
-- 

Basically just excluded any file which didn't pass checkFunc test. Should I create a PR(and fix any remaining unit tests) or maybe there's a better solution?

[samsinsane]

I think having an exception for only ExcludedFromBuild would be just as limiting as having the checkFunc not be overridable by modules. Instead, why not move all of the checkFuncs out of the call to m.emitFiles and put them into the category block. So, something like this:

m.categories.ResourceCompile = {
  name       = "ResourceCompile",
  extensions = ".rc",
  priority   = 6,

  emitFilesCheck = function(cfg)
    return cfg.system == p.WINDOWS
  end,

  emitFiles = function(prj, group)
    local fileCfgFunc = {
      m.excludedFromBuild
    }

    m.emitFiles(prj, group, "ResourceCompile", nil, fileCfgFunc, m.categories.ResourceCompile.emitFilesCheck)
  end,

  emitFilter = function(prj, group)
    m.filterGroup(prj, group, "ResourceCompile")
  end
}

This would then allow you to write the following in your module:

premake.override(premake.vstudio.vc2010.categories.ResourceCompile, "emitFilesCheck", function(oldfn, cfg)
  return cfg.system == premake.XBOXONE or oldfn(cfg)
end)

Thoughts?

[ClxS]

Hi @samsinsane

I would argue that (and #1196) isn't the best solution. The end result of that is you will still need to override those functions on a non-Windows platform. Sure, you no longer need to override just emitFiles and now only emitFilesCheck, but it still means that any category which uses a checkFunc (including ones which have yet to be introduced) need to be overridden for non-Windows builds to work. For us it's okay. We already know why .rc files aren't getting excluded, but it will just lead to many future premake users having to search these issues and come across this note saying you need to override the emitFilesCheck, rather than just have it work. For example, this does not work:

filter { "platform:Win*" }
   files { "**.rc" }
filter {}

When not on Windows, which is just confusing.

I think a good solution would be a separate fileCfgFunc which is passed for things which should always be evaluated, as exclude from build should. "CustomBuild","Midl" also have this same issue.

ETA: Also, I'm wondering what the point of the checkFunc on .rc files is in the first place. While it might not be the intention of the checkFunc, that reads that only Windows platforms are allowed to ExcludeFromBuild. Our fix for this was to override the ResourceCompile.emitFiles and remove the checkFunc since it's pointless to anyone who has setup their files and excludes correctly.

[samsinsane]

I originally thought that checkFunc was to determine if the files should be emit at all, I've reread the code and I've gone back through the history to around 2014, when a commit added this: https://github.com/premake/premake-core/blob/407861bc802f9615bcae314cb1f97f135dd2f051/src/actions/vstudio/vs2010_vcxproj.lua#L503-L513

I'm going to make the assumption that either @starkos was being extremely cautious, or vcxproj at the time only supported ExcludeFromBuild for Windows. Anyway, unless @starkos can shed some light on it's like this, I'm kind of in favour of just throwing checkFunc out entirely since there doesn't seem to be any reason why it won't emit ExcludeFromBuild but will still emit the file as ResourceCompile. @yuyoyuppe @ClxS thoughts?

[yuyoyuppe]

@starkos yeah, removing it entirely looks like a good idea and shouldn't cause any harm for CustomBuild group too(I'll double-check later). Let's wait for other contributors to comment on this and if nothing comes up - proceed with that option.

[ClxS]

I agree with removing it entirely from ResourceCompile and Midl. I might be misinterpreting the code, but I think there is also a possible issue with "CustomBuild"

m.categories.CustomBuild = {
        name = "CustomBuild",
        priority = 5,

        emitFiles = function(prj, group)
            local fileFunc = {
                m.fileType
            }

            local fileCfgFunc = {
                m.excludedFromBuild,
                m.buildCommands,
                m.buildOutputs,
                m.linkObjects,
                m.buildMessage,
                m.buildAdditionalInputs
            }

            m.emitFiles(prj, group, "CustomBuild", fileFunc, fileCfgFunc, function (cfg, fcfg)
                return fileconfig.hasCustomBuildRule(fcfg)
            end)
        end,

        emitFilter = function(prj, group)
            m.filterGroup(prj, group, "CustomBuild")
        end
    }

In this case the checkFunc mostly makes sense, it'll only apply the custom build arguments if the fileconfig has a custom build rule. There might be an issue there because exclude from build is wrapped in that same set, doesn't that mean that a CustomBuild node also wont get excluded from build when the fileconfig doesn't have a build rule? So this:

filter { "platform:Win*" }
    files { "SomeCustomBuildRuleFile" }
filter {}

Would still result in the file being not excluded on non-Windows since fcfg would be null, and fail the checkFunc.

[ClxS]

Just to confirm the above does occur like I expected and will break non-Windows builds. Our "fix" for it was to replace emitFiles with this, and update usages:

function m.emitFiles(prj, group, tag, fileFunc, alwaysEvalFileCfgFunc, fileCfgFunc, checkFunc)
    -- [[ .... ]] 
                for cfg in project.eachconfig(prj) do
                    local fcfg = fileconfig.getconfig(file, cfg)
                    p.callArray(alwaysEvalFileCfgFunc, fcfg, m.configPair(cfg))
                    if not checkFunc or checkFunc(cfg, fcfg) then
                        p.callArray(fileCfgFunc, fcfg, m.configPair(cfg))
                    end
     -- [[ ... ]]
end

m.categories.CustomBuild = {
        name = "CustomBuild",
        priority = 5,

        emitFiles = function(prj, group)
            local fileFunc = {
                m.fileType
            }

            local alwaysEvalFileCfgFunc = {
                m.excludedFromBuild,
            }

            local fileCfgFunc = {
                m.buildCommands,
                m.buildOutputs,
                m.linkObjects,
                m.buildMessage,
                m.buildAdditionalInputs
            }

            m.emitFiles(prj, group, "CustomBuild", fileFunc, alwaysEvalFileCfgFunc, fileCfgFunc, function (cfg, fcfg)
                return fileconfig.hasCustomBuildRule(fcfg)
            end)
        end,

        --[[ .....  ]]
    }

Where alwaysEvalFileCfgFunc contains functions to always execute, regardless of what checkFunc says.

[tdesveauxPKFX]

I don't think you need to add alwaysEvalFileCfgFunc as you can already do this kind of things with the existing function.

To use your example:

m.categories.CustomBuild = {
    name = "CustomBuild",
    priority = 5,

    emitFiles = function(prj, group)
        local fileFunc = {
            m.fileType
        }

        local fileCfgFunc = function(fcfg, condition)
            if fcfg and fileconfig.hasCustomBuildRule(fcfg) then
                return {
                    m.excludedFromBuild,
                    m.buildCommands,
                    m.buildOutputs,
                    m.linkObjects,
                    m.buildMessage,
                    m.buildAdditionalInputs
                }
            else
                return {
                    m.excludedFromBuild
                }
            end
        end

        m.emitFiles(prj, group, "CustomBuild", fileFunc, fileCfgFunc)
    end,

    --[[ .....  ]]
}

Didn't test it but should be fairly close if it's wrong. You can take a look at m.categories.ClCompile.

@samsinsane I don't mind removing checkFunc as I showed above you don't need it (maybe I missed something?).

[smbradley]

Did this land anywhere? We recently hit this problem too.

[yuyoyuppe]

@rellensb I've opened a PR here.

[samsinsane]

Closing as the PR has been merged.