Skip to content

Compare upper-case defines when deciding what Options.txt changed - #1420

Closed
dimensionscape wants to merge 3 commits into
HaxeFoundation:masterfrom
dimensionscape:fix/options-filter-case
Closed

dimensionscape wants to merge 3 commits into
HaxeFoundation:masterfrom
dimensionscape:fix/options-filter-case

Conversation

@dimensionscape

Copy link
Copy Markdown
Contributor

filterOptions keeps the defines that affect compilation, but matched names case-sensitively against lower-case patterns. Haxe writes defines as they were given, so -D HXCPP_M64, HXCPP_TELEMETRY or HXCPP_GC_BIG_BLOCKS never reached the comparison: toggling one recompiled only the files whose generated source happened to change and none of the runtime, leaving objects built both ways. A program built plainly and then with -D HXCPP_TELEMETRY recompiled one file and failed to link on hx::StackContext::getStackForId. It now recompiles every source, links and runs.

Defines whose spellings already matched are unaffected. A project that passes upper-case HXCPP_ defines recompiles once after updating, since the stored options now include them.

filterOptions keeps the defines that affect compilation, but matched
names case-sensitively against lower-case patterns. Haxe writes defines
as they were given, so -D HXCPP_M64, HXCPP_TELEMETRY or
HXCPP_GC_BIG_BLOCKS never reached the comparison: toggling one
recompiled only the files whose generated source happened to change and
none of the runtime, leaving objects built both ways. A program built
plainly and then with -D HXCPP_TELEMETRY recompiled one file and failed
to link on hx::StackContext::getStackForId. It now recompiles every
source, links and runs.

Defines whose spellings already matched are unaffected. A project that
passes upper-case HXCPP_ defines recompiles once after updating, since
the stored options now include them.
hxcpp_std, hxcpp_regexp, hxcpp_ssl, hxcpp_sqlite, hxcpp_mysql,
hxcpp_zip_api and hxcpp_zlib_impl compile against hxcpp's headers, whose
inline allocator depends on defines such as HXCPP_TELEMETRY,
HXCPP_GC_BIG_BLOCKS and HXCPP_M64, but only the haxe, runtime and main
groups listed Options.txt. Toggling HXCPP_TELEMETRY left these libraries
as they were last compiled. After they were rebuilt under telemetry,
turning it off failed to link: Sys, File, Socket, SSL, RegExp and the
zlib wrappers still called __hxt_gc_new and __hxt_gc_alloc.

ifExists keeps a standalone build of a library, which has no
Options.txt, working as before.
A group from an imported Build.xml is built from its own directory, so
the relative Options.txt that hxcpp_zlib_impl now lists was found by
ifExists when the XML was read but not when the group was checked:
"Could not find dependencies for hxcpp_zlib_impl : [ Options.txt ]".
The path is now made absolute when it is read. Every group that listed
an options file before is built from the start directory, where the
path resolves to the same file.
Comment thread tools/hxcpp/FileGroup.hx
{
var name = def.split("=")[0];//.toLowerCase();
// Haxe writes defines as they were given, so -D HXCPP_M64 arrives upper case
var name = def.split("=")[0].toLowerCase();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unfortunately, there doesn't seem to be any context behind why this was commented out in 7218783.

Some of those defines we check against below only ever exist in one case (and would be ignored by hxcpp in the wrong case). Rather than making this full case insensitive, maybe we should just modify those individual checks to use the correct case (e.g. check HXCPP_QUIET instead of hxcpp_quiet). I think we would just need to make sure to check both name.indexOf("hxcpp") and name.indexOf("HXCPP") since there are some flags like hxcpp_api_level and hxcpp_smart_strings.

That way we avoid introducing false positives.

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.

2 participants