Page MenuHomeWildfire Games

Fix the pkgconfig module when headers are force-included
ClosedPublic

Authored by Itms on Nov 11 2017, 6:33 PM.

Details

Summary

The pkgconfig module for premake5 has a bug, which can be reproduced by using update-workspaces.sh --with-system-mozjs38. It will add -include libraries/source/spidermonkey/some/file.h to the build options, and this build option will be in the compiler command before -include precompiled.h, which breaks PCH. The SpiderMonkey header file has to be separately detected and added to forceincludes.

Test Plan

Test update-workspaces.sh --with-system-mozjs38. You should probably run clean-workspaces.sh beforehand if you tried to reproduce the original issue.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Itms created this revision.Nov 11 2017, 6:33 PM
Vulcan added a subscriber: Vulcan.Nov 11 2017, 9:12 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content
Executing section Default...
Executing section Source...
Executing section JS...
temple accepted this revision.Nov 23 2017, 12:58 AM
temple added a subscriber: temple.

I can't speak about the code, but I experienced the bug and the patch fixed it. Thanks.

(Since upgrading to mozjs 38.8 I've been using this patch (see #3039):

Index: source/scriptinterface/ScriptTypes.h
===================================================================
--- source/scriptinterface/ScriptTypes.h	(revision 20502)
+++ source/scriptinterface/ScriptTypes.h	(working copy)
@@ -79,7 +79,7 @@
 include paths.
 #endif
 
-#if MOZJS_MINOR_VERSION != 3
+#if MOZJS_MINOR_VERSION != 3 && MOZJS_MINOR_VERSION != 8
 #error Your compiler is trying to use an untested minor version of the \
 SpiderMonkey library. If you are a package maintainer, please make sure \
 to check very carefully that this version does not change the behaviour \
Index: source/simulation2/serialization/BinarySerializer.cpp
===================================================================
--- source/simulation2/serialization/BinarySerializer.cpp	(revision 20502)
+++ source/simulation2/serialization/BinarySerializer.cpp	(working copy)
@@ -145,11 +145,7 @@
 			const JSClass* jsclass = JS_GetClass(obj);
 			if (!jsclass)
 				throw PSERROR_Serialize_ScriptError("JS_GetClass failed");
-// TODO: Remove this workaround for upstream API breakage when updating SpiderMonkey
-// See https://bugzilla.mozilla.org/show_bug.cgi?id=1236373
-#define JSCLASS_CACHED_PROTO_WIDTH js::JSCLASS_CACHED_PROTO_WIDTH
 			JSProtoKey protokey = JSCLASS_CACHED_PROTO_KEY(jsclass);
-#undef JSCLASS_CACHED_PROTO_WIDTH
 
 			if (protokey == JSProto_Object)
 			{

Don't know if that's relevant to anything, but I thought I'd mention it.)

This revision is now accepted and ready to land.Nov 23 2017, 12:58 AM
This revision was automatically updated to reflect the committed changes.