Fall back to old stack layout on stable zig #306

sporksmith · 2022-01-12T16:40:57Z

No description provided.

netlify · 2022-01-12T16:41:04Z

✔️ Deploy Preview for wasm4 ready!

🔨 Explore the source changes: b91d7df

🔍 Inspect the deploy log: https://app.netlify.com/sites/wasm4/deploys/61df336387fc470007077f6b

😎 Browse the preview: https://deploy-preview-306--wasm4.netlify.app

sporksmith · 2022-01-12T16:41:19Z

@zetanumbers

cli/assets/templates/zig/build.zig

zetanumbers

Seems fine to me!

sporksmith · 2022-01-12T17:03:13Z

Verified that the template now builds and runs on both 0.9.0 (current stable) and 0.10.0-dev.258+349a7cc27

sporksmith · 2022-01-12T17:47:02Z

@zetanumbers I went back and took your suggestion for simplifying the version check. Also wrote some tests to verify the expected behavior. PTAL

sporksmith · 2022-01-12T17:57:18Z

Actually, do we know that the fix would be backported to a 0.9.1 release? If not maybe we should just check for >= 0.10.0-dev.258

sporksmith · 2022-01-12T18:00:27Z

I don't see the fix on the 0.9.x branch. Going to add some more comments and drop the assumption that 0.9.1 will include it https://github.com/ziglang/zig/commits/0.9.x

zetanumbers · 2022-01-12T18:17:19Z

I don't see the fix on the 0.9.x branch. Going to add some more comments and drop the assumption that 0.9.1 will include it https://github.com/ziglang/zig/commits/0.9.x

https://github.com/ziglang/zig/milestone/15?closed=1

Also i am subscribed to zig releases, so i will check it anyway.

zetanumbers · 2022-01-12T17:06:57Z

cli/assets/templates/zig/build.zig

+        lib.stack_size = 14752;
+    } else {
+        // `--stack-first` option have been reenabled on wasm targets with https://github.com/ziglang/zig/pull/10572
+        std.log.warn("Zig {} doesn't support stack-first. Stack overflows will result in silent data corruption.", .{zig_version});


I think this message should say what version to use. Like "update to the latest nighlty or 0.9.1"?

Done-ish. I put in the concrete version instead of "nightly"

zetanumbers · 2022-01-12T18:21:15Z

cli/assets/templates/zig/build.zig

+    try expect(!try version_supports_stack_first(try parse("0.8.0")));
+
+    try expect(!try version_supports_stack_first(try parse("0.9.0")));
+    try expect(!try version_supports_stack_first(try parse("0.9.1")));


Suggested change

try expect(!try version_supports_stack_first(try parse("0.9.1")));

try expect(!try version_supports_stack_first(try parse("0.9.1-dev.0")));

try expect(try version_supports_stack_first(try parse("0.9.1")));

Added similar

zetanumbers · 2022-01-12T18:26:26Z

cli/assets/templates/zig/build.zig

+fn version_supports_stack_first(zig_version: std.SemanticVersion) !bool {
+    // TODO, also allow corresponding 0.9.x+ if and when the fix has been backported.
+    // As of today it hasn't been backported yet: https://github.com/ziglang/zig/commits/0.9.x
+    return zig_version.order(try std.SemanticVersion.parse("0.10.0-dev.258")).compare(.gte);


Suggested change

return zig_version.order(try std.SemanticVersion.parse("0.10.0-dev.258")).compare(.gte);

if (zig_version.pre) |pre| {

// assert we understand the format

try std.testing.expect(pre.len >= 5);

try expectEqualStrings(pre[0..4], "dev.");

try std.fmt.parseUnsigned(u32, pre[4..], 10);

return zig_version.order(try std.SemanticVersion.parse("0.10.0-dev.258")).compare(.gte);

} else return zig_version.order(try std.SemanticVersion.parse("0.9.1")).compare(.gte);

We shouldn't need to parse the pre tag. Semantic versioning already specifies an ordering over pre tags, which is implemented in the SemanticVersion class.

See 11.4 https://semver.org/#spec-item-11

Yes, however i added assertion just to make sure there's nothing like 0.10.0-bootstrap.1. Something like 0.10.0-alpha.1 would be less than 0.10.0-dev.258 by these rules, but that would be wrong, so i think we better handle only nighlty's dev-* and stable.

IMO them doing that would be a violation of the semver spec, but I don't know their processes well enough to be confident that they won't do it anyway. Went ahead and put in a check for "dev.".

sporksmith · 2022-01-12T18:41:37Z

https://github.com/ziglang/zig/milestone/15?closed=1

I see they assigned the issue to the 0.9.1 milestone, but I still don't see a merged fix in the 0.9.x branch.

Also i am subscribed to zig releases, so i will check it anyway.

I'd lean towards conservatively keeping the old behavior on 0.9.x until we see the fix merged. Wdyt?

zetanumbers · 2022-01-12T18:52:55Z

I see they assigned the issue to the 0.9.1 milestone, but I still don't see a merged fix in the 0.9.x branch.

They commit to the nightly until they need 0.9.x branch, then they will bump it up to 0.9.1. I think it would be tedious to manage one branch of an org everytime there's fix on the nightly. By your logic there's no indicaion they are working on 0.9.1 currently?

I'd lean towards conservatively keeping the old behavior on 0.9.x until we see the fix merged. Wdyt?

We are preparing for a release too, so we should look forward too.

sporksmith · 2022-01-12T18:56:37Z

Yeah, confirmed on ziglang/zig#10567 that while it hasn't been cherry-picked, it's slated to be for the 0.9.1 release.

I'd still lean toward keeping the old behavior until it's actually cherry-picked, or maybe until the release is actually cut, in the case that it doesn't get cherry-picked, or gets cherry-picked and then reverted before release for whatever reason.

Being in a situation where the tutorial code doesn't put the stack first on zig 0.9.1 even though it could is not so bad. Being in a situation where it does put the stack first on 0.9.1 but it fails in a weird way at runtime would be pretty bad.

sporksmith · 2022-01-12T19:04:18Z

Ah, they just pushed it. I'll go ahead and enable on 0.9.1

sporksmith · 2022-01-12T19:33:08Z

Oops, it's wrong for 0.10.1. Pushing One More Fix (TM)

sporksmith · 2022-01-12T19:35:04Z

Oh it was right after all; forgot I'd already covered that at the very beginning. Added a couple more test cases instead.

zetanumbers · 2022-01-12T19:41:38Z

cli/assets/templates/zig/build.zig

+    // Conservatively don't recognize tags other than 'dev'.
+    try expect(!try version_supports_stack_first(try parse("0.10.0-aev.259")));
+    try expect(!try version_supports_stack_first(try parse("0.10.0-zev.259")));


I think we should fail on exotic zig versions, then the user would handle this code themselves to fix it. We could add a recommendation on the website to download nightly or stable 0.9.1. wdyt?

I slightly prefer the current behavior of just falling back to safe legacy behavior and printing a warning. No need to force them into a session of debugging our build script to be able to build at all.

We could add a recommendation on the website to download nightly or stable 0.9.1

Probably not worth it IMO. It's another place we'd need to keep a version number up to date. "For best results use a recent version" mostly goes without saying :)

Well then, the descision is behind @aduros

zetanumbers · 2022-01-12T19:44:35Z

cli/assets/templates/zig/build.zig

+        // in case zig uses other prefixes that don't respect semver ordering.
+        if (zig_version.pre) |pre| {
+            // Merged here: https://github.com/ziglang/zig/pull/10572
+            return std.mem.startsWith(u8, pre, "dev.") and zig_version.order(try std.SemanticVersion.parse("0.10.0-dev.258")).compare(.gte);


Mentioned below. Everything else seems fine.

cli/assets/templates/zig/build.zig

Co-authored-by: ZetaNumbers <dariasukhonina@gmail.com>

zetanumbers reviewed Jan 12, 2022

View reviewed changes

cli/assets/templates/zig/build.zig Outdated Show resolved Hide resolved

zetanumbers approved these changes Jan 12, 2022

View reviewed changes

Fall back to old stack layout on stable zig

8e7160a

sporksmith force-pushed the zig-warn branch from 9b7d544 to 8e7160a Compare January 12, 2022 16:58

Simplify stack version check

8fdbf1b

Don't assume stack fix will be backported to 0.9.1

a5cfda8

zetanumbers suggested changes Jan 12, 2022

View reviewed changes

Jim Newsome added 2 commits January 12, 2022 13:21

Put stack first on 0.9.1, and check pre tag

f2b536e

Improve warning about stack-first

665fe7e

Add more tests for 0.10.x

f97e363

>= in version warning

72f27bd

zetanumbers reviewed Jan 12, 2022

View reviewed changes

zetanumbers suggested changes Jan 12, 2022

View reviewed changes

cli/assets/templates/zig/build.zig Outdated Show resolved Hide resolved

Update cli/assets/templates/zig/build.zig

b91d7df

Co-authored-by: ZetaNumbers <dariasukhonina@gmail.com>

zetanumbers approved these changes Jan 12, 2022

View reviewed changes

aduros merged commit ed7dcb5 into aduros:main Jan 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fall back to old stack layout on stable zig #306

Fall back to old stack layout on stable zig #306

sporksmith commented Jan 12, 2022

netlify bot commented Jan 12, 2022 •

edited

Loading

sporksmith commented Jan 12, 2022

zetanumbers left a comment

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

zetanumbers commented Jan 12, 2022 •

edited

Loading

zetanumbers Jan 12, 2022

sporksmith Jan 12, 2022

zetanumbers Jan 12, 2022

sporksmith Jan 12, 2022

zetanumbers Jan 12, 2022

sporksmith Jan 12, 2022

sporksmith Jan 12, 2022

zetanumbers Jan 12, 2022

sporksmith Jan 12, 2022

sporksmith commented Jan 12, 2022

zetanumbers commented Jan 12, 2022

sporksmith commented Jan 12, 2022 •

edited

Loading

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

zetanumbers Jan 12, 2022

sporksmith Jan 12, 2022

sporksmith Jan 12, 2022

zetanumbers Jan 12, 2022

zetanumbers Jan 12, 2022

	try expect(!try version_supports_stack_first(try parse("0.9.1")));
	try expect(!try version_supports_stack_first(try parse("0.9.1-dev.0")));
	try expect(try version_supports_stack_first(try parse("0.9.1")));

-    return zig_version.order(try std.SemanticVersion.parse("0.10.0-dev.258")).compare(.gte);
+    if (zig_version.pre) |pre| {
+        // assert we understand the format
+        try std.testing.expect(pre.len >= 5);
+        try expectEqualStrings(pre[0..4], "dev.");
+        try std.fmt.parseUnsigned(u32, pre[4..], 10);
+        return zig_version.order(try std.SemanticVersion.parse("0.10.0-dev.258")).compare(.gte);
+    } else return zig_version.order(try std.SemanticVersion.parse("0.9.1")).compare(.gte);

Fall back to old stack layout on stable zig #306

Fall back to old stack layout on stable zig #306

Conversation

sporksmith commented Jan 12, 2022

netlify bot commented Jan 12, 2022 • edited Loading

sporksmith commented Jan 12, 2022

zetanumbers left a comment

Choose a reason for hiding this comment

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

zetanumbers commented Jan 12, 2022 • 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

Choose a reason for hiding this comment

sporksmith commented Jan 12, 2022

zetanumbers commented Jan 12, 2022

sporksmith commented Jan 12, 2022 • edited Loading

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

sporksmith commented Jan 12, 2022

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

netlify bot commented Jan 12, 2022 •

edited

Loading

zetanumbers commented Jan 12, 2022 •

edited

Loading

sporksmith commented Jan 12, 2022 •

edited

Loading