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

Workarounds for a bug in certain versions of Mono (3.10 and certain versions of 3.8) which stops OpenRA building #6931

Merged
merged 1 commit into from Nov 12, 2014

Conversation

Happy0
Copy link
Contributor

@Happy0 Happy0 commented Nov 11, 2014

[happy0@gamma OpenRA]$ make
CSC OpenRA.Game.exe
OpenRA.Game/Exts.cs(378,6): error CS1525: Unexpected symbol `?'
Compilation failed: 1 error(s), 0 warnings
Makefile:249: recipe for target 'OpenRA.Game.exe' failed
make: *** [OpenRA.Game.exe] Error 1
[happy0@gamma OpenRA]$ mono -V
Mono JIT compiler version 3.10.0 (tarball Mon Oct 6 20:46:04 UTC 2014)

Mono bug report : https://bugzilla.xamarin.com/show_bug.cgi?id=23319 (it appears to have been resolved in master so shouldn't be present in future versions of Mono)

Mono 3.10 seems to be unhappy with ternary statements of a certain format, so I desugared those breaking the build into if statements (and OpenRA subsequently built.)

I wasn't sure whether you would accept this pull request, since I'm not sure whether changing code to work around compiler bugs is even a good idea, but I thought I'd give you guys the option anyway. Arguably the code is a bit more readable this way anyway :P. Up to you!

@Happy0 Happy0 changed the title Workarounds for a bug in Mono 3.10 which prevent OpenRA building Workarounds for a bug in Mono 3.10 which stops OpenRA building Nov 11, 2014
@Happy0 Happy0 changed the title Workarounds for a bug in Mono 3.10 which stops OpenRA building Workarounds for a bug in 64 bit versions of Mono which stops OpenRA building Nov 11, 2014
@phrohdoh
Copy link
Member

If you could leave a comment above these such as // Workaround for broken ternary in Mono 3.10 then you have my 👍

@Happy0
Copy link
Contributor Author

Happy0 commented Nov 11, 2014

Good shout. I had considered it, but worried it might make the code look less clean.

Updated with comment referring to the bug report for the mono bug.

@MatthijsBenschop
Copy link
Contributor

I dont agree with the comment, its unnecessary, the commit message is more than enough. New people looking in the code will wonder for what code it actually was a workaround for (o.0)

Also it looks way better than the code it replaces, because this way it is clear within one second what it does as oposed to the previous cryptic (o.0) construct and, as a nice bonus this one does compile on newer mono.

Back on topic: Should we make a new 20141029 release with this one cherrypicked on top of it? Maybe this bug is the one keeping arch linux from updating its package? - which in turn fragments the online community.

? ts[i, j] : t;
// Workaround for broken ternary operators in 64 bit mono: https://bugzilla.xamarin.com/show_bug.cgi?id=23319
if (i <= ts.GetUpperBound(0) && j <= ts.GetUpperBound(1))
result [i, j] = ts [i, j];
Copy link
Member

Choose a reason for hiding this comment

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

Stray spaces before [

@obrakmann
Copy link
Contributor

If this is all that it takes to make 3.10 compile again, then I'm all for it, 👍 . My impression was that 3.10 broke all ternary operators, not only some of them.

@chrisforbes
Copy link
Member

Can we lean on Xamarin / mono project a bit to get a fix for this into 3.10.1? This seems ridiculous.

👍 for the fix.

…lding on Mono 3.10 and certain versions of the 3.8 series due to a bug in Mono: https://bugzilla.xamarin.com/show_bug.cgi?id=23319
@Happy0 Happy0 force-pushed the mono310_workaround branch 2 times, most recently from f69a1b7 to 04cbea3 Compare November 11, 2014 21:46
@Happy0
Copy link
Contributor Author

Happy0 commented Nov 11, 2014

Cheers guys :D.

Made changes based on code reviews:

  • Removed rogue spaces in array indexes.
  • Made the comments for the work arounds more accurate by removing reference to 64 bit versions and mentioning 'certain versions of the 3.8 series'
  • Updated commit message with a reference to the bug report.

@MatthijsBenschop as an arch linux user, I ran into this mono bug while trying to manually compiling the project to play OpenRA since the package hasn't been updated, so this is perhaps the case.

@Happy0 Happy0 changed the title Workarounds for a bug in 64 bit versions of Mono which stops OpenRA building Workarounds for a bug in certain versions of Mono (3.10 and certain versions of 3.8) which stops OpenRA building Nov 11, 2014
obrakmann added a commit that referenced this pull request Nov 12, 2014
Workarounds for a bug in certain versions of Mono (3.10 and certain versions of 3.8) which stops OpenRA building
@obrakmann obrakmann merged commit 22c2dc5 into OpenRA:bleed Nov 12, 2014
@obrakmann
Copy link
Contributor

Changelog

@obrakmann obrakmann mentioned this pull request Nov 12, 2014
14 tasks
@Mailaender
Copy link
Member

I can confirm that this fixed the Arch Linux build. https://build.opensuse.org/package/show/games:openra/development

@Mailaender
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants