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

chore: update yggdrasil to handle latest spec #261

Merged
merged 4 commits into from
Jan 16, 2025

Conversation

sighphyre
Copy link
Member

@sighphyre sighphyre commented Jan 15, 2025

Bumps Yggdrasil to expose the core version and handle UTF-8 characters

"Fixes" a timing issue on Windows in the tests. How did we not notice that before? Well we've never run these tests on a Windows runner

@sighphyre sighphyre changed the base branch from main to yggdrasil-engine January 15, 2025 09:07
@sighphyre sighphyre force-pushed the chore/get-ygg-version-from-ygg branch from 580a66b to cbaa590 Compare January 15, 2025 09:19
@sighphyre sighphyre force-pushed the chore/get-ygg-version-from-ygg branch from cbaa590 to 8d7e5ab Compare January 16, 2025 07:08
@sighphyre sighphyre force-pushed the chore/get-ygg-version-from-ygg branch from b5db17a to 1889e0b Compare January 16, 2025 12:22
@sighphyre sighphyre force-pushed the chore/get-ygg-version-from-ygg branch from 1889e0b to 5641bf9 Compare January 16, 2025 12:28
@sighphyre sighphyre changed the title chore: get ygg version from ygg chore: update yggdrasil to handle latest spec Jan 16, 2025
@@ -310,6 +310,7 @@ public void asynchronous_fetch_on_initialisation_fails_silently_and_retries()
.build();

Unleash unleash = new DefaultUnleash(config);
Thread.sleep(1);
Copy link
Member Author

Choose a reason for hiding this comment

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

cries in Windows So this is the failing test that sent me down the rabbit hole of the client spec tests (which aren't yet fixed). It looks like there's a very subtle difference in the thread scheduler between Windows and Linux. Linux is more precise, which means that the fetch has always completed before the verify runs on the next line. On Windows, this never completes without this sleep

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can't wait on DefaultUnleash constructor, but tbh, I don't like it does fetchFeatures as part of constructing the object... I think fetch features should be part of some initialize() method and then maybe you'd be able to wait on it... but sleeping is fine, people need to sleep :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I'd like it to change too and what you propose is what I'd like to do too, Constructors should construct, not spawn threads.

Small steps though, this is legacy behaviour that's being surfaced because we're doing Windows tests now. Can fix next series of PRs

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

I like it!

@@ -310,6 +310,7 @@ public void asynchronous_fetch_on_initialisation_fails_silently_and_retries()
.build();

Unleash unleash = new DefaultUnleash(config);
Thread.sleep(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can't wait on DefaultUnleash constructor, but tbh, I don't like it does fetchFeatures as part of constructing the object... I think fetch features should be part of some initialize() method and then maybe you'd be able to wait on it... but sleeping is fine, people need to sleep :)

@sighphyre sighphyre merged commit 26d5f3d into yggdrasil-engine Jan 16, 2025
9 checks passed
@sighphyre sighphyre deleted the chore/get-ygg-version-from-ygg branch January 16, 2025 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants