Integrated Android module into vstudio module

[redorav]

What does this PR do?

Integrates VS Android more tightly into the vstudio module, along the lines of the Linux integration. It moves all the functionality of the existing module into vstudio-related files. It also fixes some issues with the previous integration such as tags and options that don't make sense for the Android project type, as well as fixing others (e.g. Android debug PDBs)

How does this PR change Premake's behavior?

It's meant to have no breaking changes. I have tested in my own project that makes use of it and I can build, deploy and debug on VS2019 with an Ant-based project (Ant-based projects were deprecated for VS022 so that would be further work to integrate Gradle projects if I can get them to work). All unit tests are passing locally too.

Anything else we should know?

The idea is to integrate core functionality as tightly as possible with Premake. I personally think the module approach is quite complicated to follow and edit, especially if they are in core already. My hope is that we can bring in other extensions into premake core

Did you check all the boxes?

• Focus on a single fix or feature; remove any unrelated formatting or code changes
• Add unit tests showing fix or feature works; all tests pass
• Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
• Follow our coding conventions
• Minimize the number of commits
• Align documentation to your changes

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

[Jarod42]

Suggested change

	Enables linker optimizations to remove unused data by emitting each data item in a separate section.
	Emit each data item in a separate section. This help linker optimizations to remove unused data.

It looks like -ffunction-sections/-fdata-sections gcc flags.

[tritao]

I wonder if a more general linksections { "data", "function" } API would be cleaner here.

[redorav]

Yes, you are correct that these map to -ffunction-sections/-fdata-sections. I have been looking to add the flags in the way you describe, and realized the default is to turn these on. The only reason I was adding this is that in the old module there was a remnant of this option and sounded useful, but given it's on by default, does it still make sense to have it? Is there any reason to turn it off?

[Jarod42]

https://stackoverflow.com/questions/4274804/query-on-ffunction-section-fdata-sections-options-of-gcc
There are some possible drawbacks

[redorav]

Ok, what kind of api would you want then? Disabled by default even though the Android default is on? How would you create an api that disables them? I think I didn't think this particular part through too much :/

I've thought about it for a bit, I think if you don't set anything, the default should apply. If you set something then you have the choice to override it. Otherwise Premake would be changing what the default is for each platform, which we might not want to do. So I'm more in favor of separate datalevellinking and functionlevellinking apis. What do you think? Or, alternatively, is it possible to do something like:

linksections { "data", "off" }
linksections { "function", "on" }

or something like that? I don't particularly mind. Or we can change the name if you like

linksectiondata ('on')
linksectionfunction('off')

What do you think?

[redorav]

I've gone ahead and added that last one, let me know

[Jarod42]

Last one seems better, but that might probably go in another PR.

[p0358]

@redorav Regression:

	vstudio.vs2010_architectures =
	{
		win32   = "x86",
	}

+	if _ACTION < "vs2015" then
+		vstudio.vs2010_architectures.android = "Android"
+	end

	local function architecture(system, arch)
		local result
		if _ACTION >= "vs2010" then
			result = vstudio.vs2010_architectures[arch] or vstudio.vs2010_architectures[system]
		end
		return result or vstudio.vs200x_architectures[arch] or vstudio.vs200x_architectures[system]
	end

this causes:

Error: [string "vstudio/vstudio.lua"]:20: attempt to compare nil with string

Util scripts can require("vstudio") at any point, in order to extend the built-in functions such as premake.override(premake.vstudio.sln2005, "projects", function(base, wks) ..., and _ACTION is apparently not something defined at this stage. But putting such a check in global seems invalid regardless, there would be circumstances otherwise where it'd be nil silently. And wouldn't vstudio.lua be re-used multiple times as a common/base script for multiple vs actions?

EDIT: With that said I apparently don't seem to be able to workaround that with:

_ACTION = nil -- workaround Premake bug
require("vstudio")

so now I'm confused, but what I can tell is that in 5.0.0-beta3 it works, in 5.0.0-beta4 it breaks

EDIT 2: I'm dumb, _ACTION = nil is defined already in base/action.lua so this was ALREADY set. I need to set _ACTION = "" for the workaround to work

[redorav]

@p0358 What kind of script causes your issue? I'm surprised _ACTION is not defined at the point where vstudio is loaded, I thought it was always set to something when you run Premake. This was ported from the Android module so perhaps it needs a different place

[redorav]

It's probably not a problem to remove it, versions earlier than VS2015 don't support Android projects properly anyway

[p0358]

To reproduce you just put require("vstudio") in premake5.lua as your first line. It's documented at: https://premake.github.io/docs/Overrides-and-Call-Arrays/#introducing-overrides but perhaps lacks a regression unit test

I would guess _ACTION is only defined at the stage where actual project files are being generated, before that it needs to run through main scripts to collect things such as user generating their own actions etc. So if something similar is in other files, it would also cause issues there as soon as someone tries to load such script earlier to override some stuff in it (and they cannot do it at the stage where Premake is already running an action, since then it might execute functions that the user wanted to override)

EDIT: The only workaround that seems to work is to do (in a subfile that's require-d itself):

local _ACTION = "" -- workaround Premake bug
require("vstudio")

I don't fully seem to get why, at least by looking at base/action.lua again, it would seem like it should try to set _ACTION immediately and regardless of whether it was set already. And yet, if I do this previous workaround thing with global _ACTION = "", even after resetting it back to nil after include or not, it'd result in two different errors (saying no action named '' and showing command line usage help message respectively).

EDIT 2: nah the above workaround is broken too

[redorav]

I've tried removing the line and one of the unit tests is failing now. I'm trying to figure out what is broken, the test or the line. It's kind of weird because we are setting the action as vs2015 in this line p.action.set("vs2015")

Yet somehow the unit test changes, which makes me think that wasn't really working correctly

[p0358]

I kinda figured out what was happening I think!

• premake5.exe --version => _ACTION = nil
• premake5.exe version => _ACTION = "version"

It's set in advance indeed, it's just that there isn't always action when a premake command is ran!

As such this is the only workaround that actually works:

if _ACTION == nil then _ACTION = "" end -- workaround Premake bug, TODO: remove
require("vstudio")
if _ACTION == "" then _ACTION = nil end -- workaround Premake bug, TODO: remove

[redorav]

I don't know how to add a unit test that checks for that as our unit tests generally check for output in a correctly generated file, I'll ask around. It's good to know you got to the bottom of it for now at least.

[p0358]

I'm trying to figure out what is broken, the test or the line. It's kind of weird because we are setting the action as vs2015 in this line p.action.set("vs2015")

local p = premake
local suite = test.declare("test_android_files")
local vc2010 = p.vstudio.vc2010


--
-- Setup
--

	local wks, prj

	function suite.setup()
		p.action.set("vs2015")
		wks = test.createWorkspace()
	end

If premake.vstudio is already defined, then you're changing the action after the global check for _ACTION from vstudio has already ran it'd seem. So you can see how this preemptive check is problematic and surprising xd I imagine the check from the vstudio ran with _ACTION equal to test

[redorav]

Yes that makes sense, it's obvious in hindsight but I'm still learning about premake. We don't care about the workaround for pre-vs2015 so I've fixed the unit tests and hopefully it can get approved soon

[p0358]

I don't know how to add a unit test that checks for that as our unit tests generally check for output in a correctly generated file, I'll ask around. It's good to know you got to the bottom of it for now at least.

If they run multiple tests within a single process where they change _ACTION multiple times (and it seems so), then nothing in globals can depend on a value of _ACTION even moreso, only stuff in functions.

But in such case indeed I also have no idea how a case like this could be covered there without spawning a new process or using some kind of separate/virtual/fake global scope... It's also non-trivial because something like this could potentially happen in any file. I think they'd just have to kinda dry-load every file to a clean premake scope and ensure it doesn't error out, including with both _ACTION being nil and non-nill, and then also set up some kind of getter proxies to raise an error if anything tries to access any global that might potentially be changing later, at the moment of loading

it's obvious in hindsight but I'm still learning about premake

No worries, I was also confused at my beginning of tinkering with premake about what happens when etc. (though I don't remember the details, I didn't end up refactoring/upstreaming/reworking/finishing everything I wanted back then sadly :/)

[redorav]

It's probably the case that existing code is already correct, I'll just have to pay more attention in the future. Thanks for reporting and sorry for the breakage!