filter with tags and files not working

[tdesveaux]

I am using the tags filter to improve readability in some of the complex filters in our projects and came accross this issue.

Here is an example using the the base example from #789.

workspace 'foobar'
    configurations { 'release-std', 'debug-std', 'release-blz', 'debug-blz' }

   filter { 'configurations:*-std' }
        tags { 'use-std' }
   filter { 'configurations:*-blz' }
        tags { 'use-blz' }

   project 'test'
	kind 'ConsoleApp'
	platforms { 'x32', 'x64' }

	files
	{
		"src/main.c",
		"src/test.c"
	}

        filter { 'tags:use-std' } -- ok
            defines { 'USE_STD' }`

        filter { 'files:main.c' }  -- ok
            defines { 'MAIN' }

        filter { 'tags:use-blz', 'files:main.c' }  -- ignored
	    defines { 'BLZ_MAIN' }

I am willing to take a look but would appreciate if someone could point me in where I should be looking.

[samsinsane]

My initial reaction would be to have this function merge tables, but I'm concerned that this could result in all sorts of unexpected behaviour. Ultimately, the issue is that this(project-level) has a higher precedence than this(configurations-level), when applied to the file-level configurations here(note that fcfg already includes the project-level filters, and it's being "merged" into the cfg filters). That might actually be the intended behaviour, but self.tags is an empty table, which is why having mergeFilters merge tables would fix this.

@starkos and/or @tvandijck would have a better idea of what is the "better" way of fixing this, I haven't really touched this section of code before, so it would be a shot in the dark.

[tvandijck]

I think this actually a bug indeed in the mergeFilters in that it assumes that values are 'nil' if not set... but for any API value that is a table, this is not true, it is an empty table instead. My guess would be that for any other value like this, the same issue would show up.

We've so far been getting away with it, because none of the other criteria values are tables, except for "options", however that is an entirely constant table since it's the command line options, it gets filled up once and never touched again.

We'd have to fix up mergeFilters to do something a little more special for the 'tags'.

[samsinsane]

"system" and "host" are also tables, on my PC I got { "windows", "win32" } for both. I wonder what would happen for NaCl or Emscripten, if the system table was merged. Would this cause NaCl and Emscripten platforms to be flagged as Windows if the project supported building for one (or both) and Windows?

[tvandijck]

Well, they are, but they are not really...
https://github.com/premake/premake-core/blob/master/src/base/oven.lua#L67
They are just made into tables based on the "cfg.system" value, and that value is not a table.

What might simply be wrong is this part:
https://github.com/premake/premake-core/blob/master/src/base/fileconfig.lua#L103-L104

Instead of merging the filters, we might actually have to create the filters like we do elsewhere.

[tvandijck]

All that said. I wish we could somehow 'hash' and 'cache' the filter contexts... In particular the once created in the fileconfig, creates 85000 contexts for our project, of which only 1000 or so are unique.

[tdesveaux]

As @tvandijck said, using table.merge instead of overriding the tags in mergeFilters solve the problem.

The fix is just an if for tags as I am worried about unwanted side effects with system.

[tdesveaux]

After taking another look, I realized that mergeFilters is not behaving as you could expect from it's name.
It will currently replace common keys between ctx and src with src values.

function context.mergeFilters(ctx, src)
	if src.terms ~= nil then
		ctx.terms = table.merge(ctx.terms or {}, src.terms)
	end
end

This implementation would fix the tags issue and does not cause any side effects as far as I know.

Am I missing something that invalidate this solution ?

[tvandijck]

If I recall correctly, that's how it used to be implemented, and it was modified to it's current implementation for performance reasons... We'd have to look in the history of that file/function to find that back however.

It probably works for most project just fine, until you throw Starcraft 2 at it ;) That said, it is obviously broken, so we should fix it, and look at deal with performance after. I would however like to see some test cases to cover the currently broken state. So we don't break it again.

[tdesveauxPKFX]

Fixed by #889