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.

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

More angelscript fixes #2211

Closed
wants to merge 3 commits into from
Closed

More angelscript fixes #2211

wants to merge 3 commits into from

Conversation

Vincent-C
Copy link

This is a followup for PR #2195; the earlier PR contributed by @jcowgill fixes build failures on most archs mentioned in the initial bug report in #2194 except for armel. This PR fixes the armel build on Debian as well (build logs can be found here). Thanks for considering!

jcowgill and others added 3 commits May 17, 2015 00:06
For the build failures on 64-bit arches, angelscript was using __LP64__ to
test for x86_64. Use the correct define instead.

For ARM architectures, ensure the native function calling file is built on all
ARM arches and not just when building for android. Also add the nesseary
-Wa,-mimplicit-it option when building the assembly file.
The assembly code will not compile on that arch.
@jcowgill
Copy link
Contributor

jcowgill commented Jun 4, 2015

Sorry about my ARMv4 patch not working.

It looks like you actually fixed the assembly to work on ARMv4 so do you need my second patch at all? Also the commit message is wrong because you ported it to ARMv4 and haven't actually disabled anything :)

@Vincent-C
Copy link
Author

No worries! Thanks for all the work you've put in to make STK compile on the other archs that were failing.

I actually haven't tested a build without your second patch yet (it took forever to finish a single build on the Debian armel porterbox I was using), but it doesn't hurt to include it. Agreed about the silly commit message though, not sure what I was thinking. :)

@auriamg
Copy link
Member

auriamg commented Jun 6, 2015

The main issue with these pull requests is that while we integrate the angelscript sourcecode into STK for convenience, we haven't actually forked angelscript nor do we intend do. So modifications to angelscript itself should be probably submitted upstream, unless there's some bits specific to the STK build system of course

@Vincent-C
Copy link
Author

Ack, I've already submitted a patch upstream (via email to the upstream author); I just wasn't sure earlier whether or not the embedded angelscript copy in STK was intended as a convenience copy or a fork. I can just keep this as a Debian-specific patch for now. Thanks anyways!

@hiker
Copy link
Contributor

hiker commented Aug 1, 2015

If the patch is useful and was submitted upstream, I think we can include it?

@Vincent-C
Copy link
Author

Just FYI, the patch was applied upstream as svn r2177 (http://sourceforge.net/p/angelscript/code/2177/)

@auriamg
Copy link
Member

auriamg commented Aug 2, 2015

Thanks! We will eventually upgrade to a newer version of angelscript and then we'll get this fix. Given that STK doesn't currently support ARM I'd say there is no urgent need to ugprade to a newer AS

@auriamg
Copy link
Member

auriamg commented Feb 18, 2016

We have just updated to a newer version of angelscript, that should hopefully address those issues. Please reopene if there are more issues

@auriamg auriamg closed this Feb 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants