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.

Sign up for GitHub

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

Android fixes from bliz #1112

Closed
wants to merge 5 commits into from
Closed

Android fixes from bliz #1112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15d6c97
small change to allow finer grained overrides of vs pch
bwhittle Feb 27, 2018
187a60f
Add basic tests for configuration properties (#13)
pdoane-blizzard Jan 25, 2018
03ef46d
Add 5.0 for clang toolsetversion (#14)
pdoane-blizzard Jan 25, 2018
2648ade
misc android fixes (#15)
bwhittle Mar 9, 2018
7ed2203
Don't output PlatformToolset when toolchainversion isn't specified (#16)
pdoane-blizzard Mar 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions modules/android/_preload.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
"3.5",
"3.6",
"3.8",
"5.0",
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see this list disappear one day, a function that checks if there's a . and that both sides contain a number should be sufficient. We just need to generate {Toolset}_{LeftNumber}_{RightNumber} - a PR into core shouldn't be required to use a different version of the toolset. I'm happy for a warning to be emit when encountering a toolset outside of this list, but only a warning not an error like we do now.

},
}

Expand Down
1 change: 1 addition & 0 deletions modules/android/tests/_tests.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ require ("android")
return {
"test_android_files.lua",
"test_android_project.lua",
"test_config_props.lua"
}
15 changes: 15 additions & 0 deletions modules/android/tests/test_android_project.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,18 @@
<AdditionalOptions>-std=c++1z %(AdditionalOptions)</AdditionalOptions>
]]
end


function suite.precompiledHeader()
location "build"
pchheader "include/foo.h"
prepare()
test.capture [[
<ClCompile>
<PrecompiledHeader>Use</PrecompiledHeader>
<PrecompiledHeaderFile>../include/foo.h</PrecompiledHeaderFile>
<Optimization>Disabled</Optimization>
<ExceptionHandling>Enabled</ExceptionHandling>
<RuntimeTypeInfo>true</RuntimeTypeInfo>
]]
end
73 changes: 73 additions & 0 deletions modules/android/tests/test_config_props.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
local p = premake
local suite = test.declare("android_config_props")
local vc2010 = p.vstudio.vc2010
local project = p.project


--
-- Setup
--

local wks, prj

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

local function prepare()
system "android"
local cfg = test.getconfig(prj, "Debug")
vc2010.configurationProperties(cfg)
end


--
-- Check the structure with the default project values.
--

function suite.structureIsCorrect_onDefaultValues()
prepare()
test.capture [[
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Android'" Label="Configuration">
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right to me, the value for $(Platform) was always x86, x64, ARM or ARM64 for me, why is it Android now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not my code! The tests run ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can Android use the arch as the platform, and not be confused with windows configurations (that are also named for the arch)?

Copy link

Choose a reason for hiding this comment

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

There's something wrong with the test cases in general. We already had test cases for rtti and there a bug fixes here specifically for rtti. Somehow the tests are working differently in isolation and it might be why the platform is weird here too.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this looks like the vs-android configuration. Microsoft uses the platform to indicate the architecture, not the system - they use ApplicationType to indicate the system. I would say that the action isn't being set correctly for the tests, it does seem to set p.action.set("vs2015") in the suite.setup() which mightn't be the correct way of doing this in the unit tests. Might be worth giving "vs2015" a search in the tests folder and seeing how it's done elsewhere. 👍

<ConfigurationType>Application</ConfigurationType>
<UseDebugLibraries>false</UseDebugLibraries>
<CharacterSet>Unicode</CharacterSet>
</PropertyGroup>
]]
end


--
-- Check the configuration type for different architectures.
--

function suite.architecture_ARM()
architecture "ARM"
prepare()
test.capture [[
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Android'" Label="Configuration">
<ConfigurationType>Application</ConfigurationType>
<UseDebugLibraries>false</UseDebugLibraries>
<CharacterSet>Unicode</CharacterSet>
</PropertyGroup>
]]
end

--
-- Check toolchainversion
--

function suite.toolchainversion_clang_5_0()
toolchainversion '5.0'
prepare()
test.capture [[
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Android'" Label="Configuration">
<ConfigurationType>Application</ConfigurationType>
<UseDebugLibraries>false</UseDebugLibraries>
<CharacterSet>Unicode</CharacterSet>
<PlatformToolset>Clang_5_0</PlatformToolset>
</PropertyGroup>
]]
end

34 changes: 24 additions & 10 deletions modules/android/vsandroid_vcxproj.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,26 +142,27 @@

if _ACTION >= "vs2015" then
local gcc_map = {
["_"] = "GCC_4_9", -- default
["4.6"] = "GCC_4_6",
["4.8"] = "GCC_4_8",
["4.9"] = "GCC_4_9",
}
local clang_map = {
["_"] = "Clang_3_8", -- default
["3.4"] = "Clang_3_4",
["3.5"] = "Clang_3_5",
["3.6"] = "Clang_3_6",
["3.8"] = "Clang_3_8",
["5.0"] = "Clang_5_0",
}

local map = iif(cfg.toolset == "gcc", gcc_map, clang_map)
local ts = map[cfg.toolchainversion or "_"]
if ts == nil then
p.error('Invalid toolchainversion for the selected toolset (%s).', cfg.toolset or "clang")
end
if cfg.toolchainversion ~= nil then
local map = iif(cfg.toolset == "gcc", gcc_map, clang_map)
local ts = map[cfg.toolchainversion]
if ts == nil then
p.error('Invalid toolchainversion for the selected toolset (%s).', cfg.toolset or "clang")
end

_p(2, "<PlatformToolset>%s</PlatformToolset>", ts)
vc2010.element("PlatformToolset", nil, ts)
end
else
local archMap = {
arm = "armv5te", -- should arm5 be default? vs-android thinks so...
Expand Down Expand Up @@ -278,6 +279,12 @@
if cfg.system == p.ANDROID then
local opts = cfg.buildoptions

if cfg.disablewarnings and #cfg.disablewarnings > 0 then
for _, warning in ipairs(cfg.disablewarnings) do
table.insert(opts, '-Wno-'..warning)
end
end

local cpp_langmap = {
["C++17"] = "-std=c++1z",
["gnu++17"] = "-std=gnu++1z",
Expand Down Expand Up @@ -314,7 +321,7 @@
premake.override(vc2010, "exceptionHandling", function(oldfn, cfg, condition)
if cfg.system == p.ANDROID then
-- Note: Android defaults to 'off'
if cfg.exceptionhandling then
if cfg.exceptionhandling and cfg.exceptionhandling ~= premake.OFF then
if _ACTION >= "vs2015" then
vc2010.element("ExceptionHandling", condition, "Enabled")
else
Expand All @@ -329,14 +336,21 @@
premake.override(vc2010, "runtimeTypeInfo", function(oldfn, cfg, condition)
if cfg.system == premake.ANDROID then
-- Note: Android defaults to 'off'
if cfg.rtti then
if cfg.rtti and cfg.rtti ~= premake.OFF then
vc2010.element("RuntimeTypeInfo", condition, "true")
end
else
oldfn(cfg, condition)
end
end)

premake.override(vc2010, "precompiledHeaderFile", function(oldfn, fileName, cfg)
if cfg.system == premake.ANDROID then
vc2010.element("PrecompiledHeaderFile", nil, "%s", path.rebase(fileName, cfg.basedir, cfg.location))
else
oldfn(fileName, cfg)
end
end)

--
-- Extend Link.
Expand Down
5 changes: 4 additions & 1 deletion modules/vstudio/vs2010_vcxproj.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2263,6 +2263,9 @@
end
end

function m.precompiledHeaderFile(fileName, cfg)
m.element("PrecompiledHeaderFile", nil, "%s", fileName)
end

function m.precompiledHeader(cfg, condition)
local prjcfg, filecfg = p.config.normalize(cfg)
Expand All @@ -2275,7 +2278,7 @@
else
if not prjcfg.flags.NoPCH and prjcfg.pchheader then
m.element("PrecompiledHeader", nil, "Use")
m.element("PrecompiledHeaderFile", nil, "%s", prjcfg.pchheader)
m.precompiledHeaderFile(prjcfg.pchheader, prjcfg)
else
m.element("PrecompiledHeader", nil, "NotUsing")
end
Expand Down