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

Nightly Haxe issues #1763

Closed
Geokureli opened this issue Feb 23, 2024 · 17 comments · Fixed by #1764
Closed

Nightly Haxe issues #1763

Geokureli opened this issue Feb 23, 2024 · 17 comments · Fixed by #1764

Comments

@Geokureli
Copy link
Contributor

Geokureli commented Feb 23, 2024

Just mentioning this here in case it doesn't solve itself.

Flixel CI has started failing on nightly haxe versions due to lime's shadowed implementation of haxe.io.Bytes with the following errors:

Cpp/Neko:

/.../_internal/macros/AssetsMacro.hx:41: characters 26-33 : Too many arguments

HL:

/.../AssetsMacro.hx:78: characters 12-19 : hl.Bytes should be Int for optional function argument 'length'

HTML5:

/.../AssetsMacro.hx:37: characters 12-26 : js.lib.ArrayBuffer should be Int for optional function argument 'length'

the OG haxe.io.Bytes was changed for the first time in 5 years, all they did was remove #if cs sections, so I'm not sure why that would cause these specific errors

My main concern is that we don't really know why lime has it's own Bytes class, so if things do change with Bytes, lime will need to manually make those same changes. I wonder if it's simply better to create a lime.io.Bytes and use that? I also don't see why lime's Bytes has a different constructor for js

@Geokureli
Copy link
Contributor Author

Also note: haxe.Timer is also shadowed, according to Zeta: "the proper way to make Timer work would be to integrate the haxe event loop into the Lime event loop." here's how heaps does it

@joshtynjala
Copy link
Member

haxe.Timer is also shadowed

I noticed this one recently too. In fact, I discovered that Lime's custom haxe.Timer implementation causes openfl.utils.Timer to behave differently than flash.utils.Timer. Lime limits haxe.Timer to the frame rate, but flash.utils.Timer is allowed to update more frequently than flash.events.Event.ENTER_FRAME is dispatched.

In Flash, you can set the stage frame rate to a low value, like 1 FPS, but then call TimerEvent.updateAfterEvent() to force a rendering update on demand. Because Lime and OpenFL's Timers are limited to the frame rate, this (admittedly advanced) use case is not possible in OpenFL, like it is in Flash.

@player-03
Copy link
Contributor

player-03 commented Feb 24, 2024

So I think the immediate problem is that Lime's Bytes implementation checks #if js to determine what constructor to use, but AssetsMacro checks #if html5. I don't know why this is coming up now in particular, but it was only a matter of time.


I went back to when Lime's version of the class was added. The commit created the JS-only version of the class with a bunch of differences copied over Haxe's JS-specific version of the class.

@player-03
Copy link
Contributor

I went through the various versions of haxe.io.Bytes to find everything Lime changed. I left out changes that have no effect (e.g., whitespace changes), and ones where Haxe's version is more up-to-date.

  • Lime adds @:autoBuild metadata to all versions of the class other than the Eval one. (I believe we could accomplish the same using --macro Compiler.addMetadata().)
  • [C++] Lime replaces #if cpp with #if (cpp || webassembly).
  • [JS] Lime supports the lime_bytes_length_getter flag to allow setting length. This flag appears to be used by openfl-js, which documents the functionality. (We could patch this via macro if it's important.)
  • [JS] Lime adds some & 0xFF operations. (The Haxe devs have gone on record saying this isn't needed.)

That leaves WebAssembly as the tricky part. If not for that, we could simply remove Lime's version of the file.

Perhaps we could override Bytes.hx specifically for that target? Point to "templates/webassembly/src" as an additional source directory, and add a custom haxe/io/Bytes.hx in there? Not a huge fan of the idea, though. We'd probably forget it was there, and it'd fall even further behind.

@player-03
Copy link
Contributor

I don't know why this is coming up now in particular, but it was only a matter of time.

There was a change to the macro context just a couple days ago. Something to do with caching. Perhaps you were never supposed to be able to check #if html5 during a macro, but Lime got away with it because it happened to be cached.

@Geokureli
Copy link
Contributor Author

in the short term, does this mean that a quick fix is to change AssetMacro's #if html to #js? would this create issues for other js targets like node or something?

@player-03
Copy link
Contributor

does this mean that a quick fix is to change AssetMacro's #if html to #js?

It turns out that won't work. See my PR for details.

@player-03
Copy link
Contributor

(We could patch this via macro if it's important.)

Actually it seems we can't. Classes marked @:coreApi are checked afterwards, and if any API changes are detected, compilation fails.

If we wait until runtime, there's an alternate technique that does work.

That said, I'm starting to doubt that Bytes ever actually needed a setter. Only ByteArray is documented as having a setter, and it's an abstract, so there's no patching needed.

@player-03
Copy link
Contributor

player-03 commented Feb 28, 2024

Merged. If you set up the tests to get OpenFL Lime from Git, they should hopefully work again. Let us know if not!

@Geokureli
Copy link
Contributor Author

Geokureli commented Mar 13, 2024

Merged. If you set up the tests to get OpenFL Lime from Git, they should hopefully work again. Let us know if not!

Seems Flixel CI is still failing with nightly haxe even with lime 8.1.2

/haxelib/lime/8,1,2/src/lime/_internal/macros/AssetsMacro.hx:36: characters 32-39 : Too many arguments
/haxelib/lime/8,1,2/src/lime/_internal/macros/AssetsMacro.hx:41: lines 41-44 : Missing super constructor

@player-03
Copy link
Contributor

I can't reproduce this locally, can you?

@Geokureli
Copy link
Contributor Author

I wonder if it's linux specific, since Flixel CI uses linux. I'll try on my dual-boot laptop

@player-03
Copy link
Contributor

I run Linux.

@Geokureli
Copy link
Contributor Author

Just to be certain, can you check this and call out any differences or mistakes? also are you using the develop branch or the actual release?

I'll report back after trying nightly with lime 8.1.2, in a bit

@player-03
Copy link
Contributor

Oh interesting, there's still a difference between Haxe 4.3.4 and nightly. I'll keep digging.

@player-03
Copy link
Contributor

Ok, I don't fully understand what's going wrong, but the nightly build definitely changes something. I tried hard-coding different super() calls, and none of them worked. If we pass two arguments, that's too many arguments. If we pass one argument, it isn't enough arguments and it expected a different type. Clearly it's somehow using both the html5 version of Bytes and the hl version, at different points in the compilation. It shouldn't be doing this, but to be fair, we shouldn't have shadowed the class.

#1765 fixes the issue by removing our version of the class and falling back to the standard library's version. Fortunately, though unsurprisingly, the Haxe devs made sure the standard library works.

We'll have to merge that PR by the time Haxe 5 comes out.

@Geokureli
Copy link
Contributor Author

Thanks for all you've done!

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 a pull request may close this issue.

3 participants