Travis CI Fixes #1077

RobertBColton · 2017-09-28T00:52:41Z

Our goal with this PR is to get all the Travis builds passing.

Fixed one issue caused by a template declaration in the spline drawing functions. The container is used as a pointer so I just had to add a * and it continued to work.
Each compiler now gets its own objects folder eliminating the need to rebuild on a simple compiler switch.
Fixes typo in clang32.ey where ldlags should have been ldflags
Fixed GTK+ by copying the CFLAGS override for CXXFLAGS
Fixed the Matrix Math extension by renaming the folder from Matrix to MatrixMath which matches the Identifier: specified in the About.ey file. This extension actually had the problem since it was created but it only surfaced after my changes in commit 73a7170 to make the plugin consistent about which field is human-readable and which is not.
Box2D just needed the same fix as #5 to rename the folder from Box2D to Box2DPhysics so that it matched the identifier.
Cleaned up the async extension formatting and fixed a bug that caused travis to fail. I was statically initializing the async_load global which the DataStructures extension doesn't support because it also uses a statically initialized map for storing the actual datastructures themselves. Hence, I just moved the ds_map_create call into createThread so it's not created until it's first needed. The extension seems to be working again and I tested the events. I also removed the DataStructures extension from the Asynchronous extension's makefile because that's not correct. What we need is better UI support and to use the Depends: field correctly. For example, when enabling the Asynchronous extension a dialog would ask if you also want to enable the DataStructures extension since it depends on it. Additionally, I changed the travis yml file to only test Asynchronous extension with DataStructures extension also enabled, in the same way that it tests the JSON extension.
Made the JSON extension build again, but it looks like it never worked to begin with. I have no idea what @fundies and @sssstest were smoking at the time but it clearly didn't work for them. Removed also the flags that were enabling exceptions, because ENIGMA does not do exceptions.
Fixed some errors in the Berkeley sockets networking system related to recvfrom and getsockname casts. On Windows they take an int and on Linux they take socklen_t.
Cleans up the None audio system and provides two stubs for GM5Compat extension to build with it.
Fixes the Bullet extension makefile similar to the GTK one so that it properly finds the headers using the pkg config.

Miscellaneous Changes

Enables Unicode for Windows but it will be difficult to complete because Windows is still in the dark ages. For now, we created a helper for converting the strings and used it to make show_message support UTF-8, which is a start to fixing Win32 platform has incomplete Unicode support [u524] #1074. Other dialogs and such will also need to make use of this helper. I want to note that defining UNICODE did not automatically map MessageBox to MessageBoxW so I had to explicitly change it to MessageBoxW.
Changed the get_login function to use the '\0' null-terminator for delimiting username and password. This is pretty standard and means the user can now use vertical bar '|' in their username or password.
Expanded the None widget system to include get_login, get_integer, get_string, and show_question functions. This was necessary to get the Async extension building in Travis.

@fundies

1) Cleaned up all of the about files to have the correct specificity. Windows should now be selected as the correct default platform when on Windows, and OpenGL1.1 should be the default graphics everywhere, since again, we consider it most stable. 2) Reverted @fundies change to the ENIGMASystem/SHELL/Makefile because what @JoshDreamland had before was correct. This didn't even compile and broke Windows which was reported in #1033 that we couldn't figure out. 3) Added follow_object to roomsystem.h so the graphics systems would compile again.

RobertBColton · 2017-09-28T05:32:52Z

@JoshDreamland for #4 could you tell me if I should have just deleted CFLAGS in 7b22590 since we only use g++ now? Or is it fine since it's not hurting anything?

RobertBColton · 2017-09-28T05:59:53Z

@JoshDreamland Yeah I'd like you to review commit 73a7170 just to make sure my changes were logical. This is related to #5 but regardless of whether I need to go back and undo those earlier changes, this change is still needed to match the folder name with the id/identifier. Please see the related forum topic too where I discuss the changes that I had made at the time to the plugin:
http://enigma-dev.org/forums/index.php?topic=2718

I just want those changes audited.

…tension dependency

…robably going to have to throw it in the trash anyway, rinse, replace, and repeat

…o build

…nc extension and change get_login to use null terminator for deliminting username and password

…g literal which was causing warning, and fixed WIN_NT redefinition warning

…written in C++

JoshDreamland · 2017-09-29T05:15:54Z

CompilerSource/compiler/compile.cpp

@@ -105,29 +105,28 @@ int lang_CPP::compile(EnigmaStruct *es, const char* exe_filename, int mode)
  ide_dia_clear();
  ide_dia_open();
  cout << "Initialized." << endl;
+  string compilepath = CURRENT_PLATFORM_NAME "/" + extensions::targetOS.builddir + "/" + extensions::targetOS.target;


We can probably get away with just the current platform name and the target ey file name, but this should be fine.

Leave it as is, people using the cross-compiler will have to rebuild less, am I correct in thinking so?

JoshDreamland · 2017-09-29T05:17:12Z

CompilerSource/settings-parse/parse_ide_settings.cpp


  const char *a = establish_bearings(cinffile.c_str());
  if (a) cout << "Parse fail: " << a << endl;

  // Read info about the compiler
  ifstream cinfstream(cinffile.c_str());
  ey_data cinfo = parse_eyaml(cinfstream,cinffile);
+  extensions::targetOS.target    = target;


Read my next comment. Whatever name you pick, use it here, too.

JoshDreamland · 2017-09-29T05:18:09Z

CompilerSource/settings.h

@@ -43,7 +43,7 @@ namespace extensions
      windowLinks, graphicsLinks, audioLinks, collisionLinks, widgetLinks, networkLinks;
  };
  struct compiler_descriptor {
-    string identifier, resfile, buildext, buildname, runprog, runparam, builddir;
+    string target, identifier, resfile, buildext, buildname, runprog, runparam, builddir;


Could you rename this to target_compiler or compiler_descriptor_name or something? It's not clear what it means, as it is.

How about just "compiler" which I prefer because it's consistent with the other fields?
f63701c

JoshDreamland · 2017-09-29T05:27:18Z

ENIGMAsystem/SHELL/Universal_System/Extensions/Asynchronous/ASYNCdialog.cpp

+  ds_map_replaceanyway(async_load, "id", md->id);
+  string ret = threads[md->id]->ret;
+  size_t end = ret.find('\0', 0);
+  ds_map_replaceanyway(async_load, "username", ret.substr(0, end));


This is a problem if the string is empty.

JoshDreamland · 2017-09-29T05:33:44Z

ENIGMAsystem/SHELL/Widget_Systems/None/nowidget_impl.cpp

@@ -25,6 +25,49 @@ using std::string;

 #include "GameSettings.h"

+#include <iostream>


This change is sufficiently complex to warrant updating the copyright headers. Just extend the years to today under both our names.

Agreed. d18de1a

JoshDreamland · 2017-09-29T05:36:12Z

ENIGMAsystem/SHELL/Widget_Systems/Win32/dialogs.cpp

@@ -203,6 +204,21 @@ static INT CALLBACK GetDirectoryAltProc(HWND hwnd, UINT uMsg, LPARAM lp, LPARAM
  return 0;
 }

+static wchar_t* string_to_widechar(string str) {
+  // Number of shorts will be <= number of bytes; add one for null terminator


Thank you for preserving this comment; I forgot it. Also, maybe update these copyright headers, too. Not as important. Also also, it'll probably be good to move this method somewhere common in the future, since all of Win32 is gonna want to be using it.

Agreed. d18de1a

We can leave the helper for now, it's ok there. TKG is interested in that helper function for some things and he might be willing to do the work to implement it in the rest of our Win32 systems.

Edit: I actually should have deleted that and moved the comment... 06b1918

JoshDreamland · 2017-09-29T05:37:55Z

And just like that, we have our first green build, in the history of this project. Wow, what a fun day this has been. Good job; this has to be the most substantial positive change the project has seen in years. Thank you again.

…anges

RobertBColton · 2017-09-29T07:04:29Z

I'm just proud of the fact that we actually fixed everything instead of a cop out where we just disabled the ones we couldn't get working. We stuck it out and got them ALL working. 👍

…(which is the case if you hit cancel)

JoshDreamland

Oop, missed this change.

JoshDreamland · 2017-09-29T15:21:37Z

ENIGMAsystem/SHELL/Universal_System/Extensions/Asynchronous/ASYNCdialog.cpp

-  ds_map_replaceanyway(async_load, "password", ret.substr(end + 1, ret.size() - end));
+  vector<string> split = string_split(ret, '\0');
+  // must still check if the size is larger than 0 for when user cancels the dialog
+  ds_map_replaceanyway(async_load, "username", (split.size() > 0) ? split[0] : "");


This actually has the same problem; it's just more direct about it. Just put back what you had, but explicitly check end == string::npos and just return failure (or empty password).

RobertBColton · 2017-09-29T06:55:58Z

ENIGMAsystem/SHELL/Widget_Systems/Win32/dialogs.cpp

+  // Number of shorts will be <= number of bytes; add one for null terminator
+  const size_t wchar_count = str.size() + 1;
+  vector<WCHAR> buf(wchar_count);
+  return tstring{buf.data(), MultiByteToWideChar(CP_UTF8, 0, str.c_str(), -1, buf.data(), wchar_count)};


@JoshDreamland I don't want to forget to tell you that @time-killer-games gets a warning on this line about narrowing conversion. We overlooked that tstring takes size_t and MultiByteToWideChar returns int. We should address this before we merge.

The narrowing is in wchar_count. If we want to be all proper, we can do something like this:

# ifdef DEBUG_MODE if (wchar_count > std::numeric_limits<int>::max) show_error("Message characters will be discarded.", false); # endif

Then just cast wchar_count to (int).

In my opinion, that warning is kind of silly; it's hardly worth the code to generate it, because this only becomes relevant if the message is larger than two gigabytes. So I don't care if you want to include it or not. Go ahead and perform the cast either way, though.

If we also need to cast the int→size_t conversion, you can go ahead and store the return value in an int, verify it's greater than zero, then use it, else show a similar error. Again, I'm not opposed to this, but it should be pointless, as the return value is always zero or else the number of characters written.

Ok I tried casting it to int, that doesn't fix the warning:

Widget_Systems/Win32/dialogs.cpp: In function 'tstring widen(const string&)': Widget_Systems/Win32/dialogs.cpp:212:102: warning: narrowing conversion of 'MultiByteToWideChar(65001u, 0ul, (& str)->std::basic_string<_CharT, _Traits, _Alloc>::c_str<char, std::char_traits<char>, std::allocator<char> >(), -1, buf.std::vector<_Tp, _Alloc>::data<wchar_t, std::allocator<wchar_t> >(), ((int)wchar_count))' from 'int' to 'std::basic_string<wchar_t>::size_type {aka unsigned int}' inside { } [-Wnarrowing] return tstring{buf.data(), MultiByteToWideChar(CP_UTF8, 0, str.c_str(), -1, buf.data(), wchar_count)}; ^

Ok, I got it, I added both just in case:
82a7820
f8c34b4

RobertBColton · 2017-09-29T08:30:51Z

ENIGMAsystem/SHELL/Universal_System/Extensions/Asynchronous/ASYNCdialog.cpp

+  vector<string> split = string_split(ret, '\0');
+  // must still check if the size is larger than 0 for when user cancels the dialog
+  ds_map_replaceanyway(async_load, "username", (split.size() > 0) ? split[0] : "");
+  ds_map_replaceanyway(async_load, "password", (split.size() > 1) ? split[1] : "");


@JoshDreamland from now on, please start a review instead of a single comment, because once I push changes it won't let me go back and reply. Anyway, good thing you caught that because that is actually the case when the user cancels the dialog, the return is an empty string. So that was a regression and another segfault I managed to fix.

Are you sure there isn't perhaps a better ASCII symbol to delimit the username and password? I'm concerned that people will have a hard time splitting the strings in userland.
http://ascii-table.com/ascii.php

I forgot that I had left string_split in there, so I just came up with this. How does it look? From my testing it behaves exactly the same as GMS, and yes the event should still be fired even if the user cancels the dialog, at least by their standards.

If I had my way it probably just wouldn't fire the event when dialogs are canceled or at least tell you the return status of the dialog. I suppose I could implement the latter for all of these without breaking GM compatibility by just adding some global constants mapped to the standard dialog options. Write the return status to the map. I believe the other dialog functions that return the option just return you the string with the text of the option, so maybe that would be inconsistent.

The only difference here is that mine don't have a buffer overflow when you enter a username but not a password, which is a bug in GMS 1.4 that nobody has reported 😄

While I am at it, do you care to know why I added ds_map_replaceanyway? It's because in GMS they changed ds_map_replace to insert the key anyway if it didn't previously exist. I believe I tested ds_map_replace in GM8 and it behaved the way ENIGMA currently behaves, and not the way GMS does. I don't know if we should go back and fix that, if you think the GMS way is better. I just didn't want to break any games.

I think you'll find I did start a review. You'll get used to the new review system with time; it's a little janky. As I said, it seems to be based on review tools such as Critique at Google, but it's missing a few features that make Critique so usable. I expect them to fill in over time.

I left a comment on your string_split solution—the vector will "contain" only one element if there's no null in the string, so your code only survives because there's no error reporting mechanism. If you enabled STL's debug mode, you'd find it suddenly explodes. And rightfully so—you're directly accessing vec[0] and vec[1]. From now on, any time you see a literal array access like those, I want you to panic inside until you verify that they're guarded by something like if (vec.size() > 1). Which yours aren't.

It's more natural (and more efficient) to protect your find()-based solution, so I recommend doing that. As for users, it might be confusing to them at first, I agree. I wanted to solve this with a proper array type in ENIGMA. It could be the return type of this method, and var could have a proper constructor for it. Alternatively, we can drop GM6's behavior of var = var performing a shallow copy. If not for that design decision, var would be a perfectly fine mechanism for returning tuples.

I think the GM:S behavior of replace is better, and am happy to migrate to it. There's probably some application that depends on the old behavior, but it's not a very defensible behavior.

Josh, mine does already work that way, look again: (split.size() > 1) ? split[1] : ""

Anyway, I'm going to change it just because the other split method is less verbose and more straight forward.

I have no opinion on preserving the GM6 behavior because I don't know how widespread that behavior was relied upon. I doubt that it was relied upon very heavily being that GM6 lacked complex abstract types like classes and structs. But it seems like having var pass-by-reference would be more efficient, I mean, the same reason that Java does it for objects.

Here's a little test for the ds_map_replace thing:

mymap = ds_map_create(); ds_map_replace(mymap, "test", 5); show_message(string(ds_map_find_value(mymap, "test")));

GM5 on my pc shows "0"
ENIGMA shows empty dialog
GM:S shows "5"

So do you want to switch to the GM:S behavior? Are you sure nothing will break? It don't seem like anything could have been relying on the GM5 behavior.

Taken care of, we'll get to ds_map_replaceanyway in a separate PR.
ab11647
2effede

…because messages should generally not exceed 2gb's

JoshDreamland · 2017-09-30T02:09:35Z

So... build's green.

RobertBColton added 8 commits September 27, 2017 16:22

Fix template declaration

7c7a419

Merge branch 'master' of https://github.com/enigma-dev/enigma-dev

d5cd8ce

Add separate objects folder for each compiler

1fef22a

travis test ldflags

c72ba72

fixes typo in clang32.ey

bc64c9d

test adding gtk2.0 include dir to gtk+ makefile

9ad1b5a

fundies says try this for gtk instead

7b22590

fix matrix folder name to match matrixmath identifier in about.ey

81aa3b9

RobertBColton added 8 commits September 28, 2017 02:18

rename Box2D folder to Box2DPhysics to match extension identifier

55c8610

Fix static initialization of async_load map in extension and cleanup

f2c2a91

no needaspace

4096542

modify travis file to test async extension with its datastructures ex…

777b22e

…tension dependency

cleaned up the JSON extension, just pretend it fucking works, we're p…

7b716d4

…robably going to have to throw it in the trash anyway, rinse, replace, and repeat

delete those old fags, enigma dun do exceptions sayeth josh

551f9eb

Implements UTF-8 support for show_message on Win32

bc3dbb3

Josh says I don't need any of this winmain shit

efe16dc

RobertBColton mentioned this pull request Sep 28, 2017

Win32 platform has incomplete Unicode support [u524] #1074

Open

RobertBColton added 10 commits September 28, 2017 19:05

fix macro using void() like Josh said

b7c2225

fix recvfrom call in berkeley sockets

c604ef9

I didn't do what he said

75ec649

same story as the other one

7e111d2

cleans up none audio system and adds shells for GM5Compat extension t…

abbadaf

…o build

cleanup bullet extension

e78c667

move bullet first for a second

88af886

cleanup bullet and gtk makefiles to not use pkg config on win32

96a97bd

add additional dialog functions to none widget system for testing asy…

d42d83a

…nc extension and change get_login to use null terminator for deliminting username and password

attempted to fix berkeley sockets, removed null-terminator from strin…

a1c3540

…g literal which was causing warning, and fixed WIN_NT redefinition warning

RobertBColton added 4 commits September 29, 2017 00:22

we need the .h for c string functions for now, this file should be re…

213e474

…written in C++

BSsocket.cpp needs it too for memset

6cebc1d

he doesn't know what i'm panicking about

fbd26df

cleanup travis file and add safer string widening to win32 widgets

e107274

JoshDreamland reviewed Sep 29, 2017

View reviewed changes

RobertBColton added 3 commits September 29, 2017 01:52

use something a little more descriptive than 'target' like 'compiler'

f63701c

update copyright headers for two files that had rather significant ch…

d18de1a

…anges

totally forgot i can just delete the old one

06b1918

correct get_login_async string splicing for when the string is empty …

4290126

…(which is the case if you hit cancel)

JoshDreamland approved these changes Sep 29, 2017

View reviewed changes

JoshDreamland requested changes Sep 29, 2017

View reviewed changes

RobertBColton commented Sep 29, 2017

View reviewed changes

RobertBColton added 2 commits September 29, 2017 16:57

fix narrowing conversion warning, we don't care about this narrowing …

82a7820

…because messages should generally not exceed 2gb's

add this one too just in case

f8c34b4

JoshDreamland approved these changes Sep 29, 2017

View reviewed changes

RobertBColton added 2 commits September 29, 2017 17:47

use the other split code but fix segfault because it's less verbose

ab11647

let's clean that up a bit

2effede

JoshDreamland merged commit b173f92 into enigma-dev:master Sep 30, 2017

This was referenced Sep 30, 2017

And this happened.. enigma-dev/lgmplugin#52

Closed

Cant open ENIGMA because of LGM enigma-dev/lgmplugin#34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Travis CI Fixes #1077

Travis CI Fixes #1077

RobertBColton commented Sep 28, 2017 •

edited

Loading

RobertBColton commented Sep 28, 2017

RobertBColton commented Sep 28, 2017 •

edited

Loading

JoshDreamland Sep 29, 2017

RobertBColton Sep 29, 2017 •

edited

Loading

JoshDreamland Sep 29, 2017

JoshDreamland Sep 29, 2017

RobertBColton Sep 29, 2017

JoshDreamland Sep 29, 2017

JoshDreamland Sep 29, 2017

RobertBColton Sep 29, 2017

JoshDreamland Sep 29, 2017

RobertBColton Sep 29, 2017 •

edited

Loading

JoshDreamland commented Sep 29, 2017

RobertBColton commented Sep 29, 2017 •

edited

Loading

JoshDreamland left a comment

JoshDreamland Sep 29, 2017

RobertBColton Sep 29, 2017

JoshDreamland Sep 29, 2017

RobertBColton Sep 29, 2017

RobertBColton Sep 29, 2017 •

edited

Loading

RobertBColton Sep 29, 2017

JoshDreamland Sep 29, 2017

RobertBColton Sep 29, 2017

RobertBColton Sep 29, 2017

JoshDreamland commented Sep 30, 2017

		@@ -25,6 +25,49 @@ using std::string;

		#include "GameSettings.h"

		#include <iostream>

Travis CI Fixes #1077

Travis CI Fixes #1077

Conversation

RobertBColton commented Sep 28, 2017 • edited Loading

Miscellaneous Changes

RobertBColton commented Sep 28, 2017

RobertBColton commented Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

RobertBColton Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertBColton Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

JoshDreamland commented Sep 29, 2017

RobertBColton commented Sep 29, 2017 • edited Loading

JoshDreamland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertBColton Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshDreamland commented Sep 30, 2017

RobertBColton commented Sep 28, 2017 •

edited

Loading

RobertBColton commented Sep 28, 2017 •

edited

Loading

RobertBColton Sep 29, 2017 •

edited

Loading

RobertBColton Sep 29, 2017 •

edited

Loading

RobertBColton commented Sep 29, 2017 •

edited

Loading

RobertBColton Sep 29, 2017 •

edited

Loading