Android fixes from bliz #1112
Android fixes from bliz #1112
Conversation
| @@ -65,6 +65,7 @@ | |||
| "3.5", | |||
| "3.6", | |||
| "3.8", | |||
| "5.0", | |||
There was a problem hiding this comment.
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.
| function suite.structureIsCorrect_onDefaultValues() | ||
| prepare() | ||
| test.capture [[ | ||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Android'" Label="Configuration"> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Not my code! The tests run ;)
There was a problem hiding this comment.
How can Android use the arch as the platform, and not be confused with windows configurations (that are also named for the arch)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 👍
|
@tvandijck It was getting out of hand... something had to be done >_< |
|
@TurkeyMan have you had a chance to fix this up? |
TurkeyMan
force-pushed
the
android_fixes
branch
3 times, most recently
from
a0df305 to
861982c
Compare
* fixed exception handling and rtti 'Off' setting ignored warnings now auto generated additional build params as such added support for pch (which need to take a relative path from cwd) instead of a file located via the search path. * added unit test for android pch
TurkeyMan
force-pushed
the
android_fixes
branch
from
861982c to
7ed2203
Compare
|
I think this one needs to be closed. It looks like some of the issues it addresses have already been fixed in other PRs, and there hasn't been any movement here. Happy to see this one re-opened, but the individual fixes really should be broken out into separate PRs so they can be approved independently. Going forward, I'll be encouraging more pushback in that direction. |
No description provided.