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

update to new module system #1698

Merged
merged 4 commits into from
Jan 5, 2024
Merged

Conversation

xdBronch
Copy link
Contributor

@xdBronch xdBronch commented Jan 3, 2024

breaking changes from ziglang/zig#18160
made corresponding prs in known-folders and diffz, iiuc the hashes i used in the zon will still work once the prs are merged, github magic or whatever :p

@xdBronch
Copy link
Contributor Author

xdBronch commented Jan 3, 2024

forgot to update the min zig version of course lol

@xdBronch
Copy link
Contributor Author

xdBronch commented Jan 3, 2024

also there seems to be a regression relating to build runners being cached so zls cant really use its build runner correctly meaning module imports are broken
ziglang/zig#18438
fixed

build.zig Outdated Show resolved Hide resolved
build.zig Outdated Show resolved Hide resolved
src/build_runner/master.zig Outdated Show resolved Hide resolved
@xdBronch
Copy link
Contributor Author

xdBronch commented Jan 5, 2024

oh thats funny, the changes to the expectEqual function meant to make them better to work with are actually causing CI to fail

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (5c0bebe) 76.12% compared to head (9fbd994) 76.14%.

Files Patch % Lines
src/DocumentStore.zig 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1698      +/-   ##
==========================================
+ Coverage   76.12%   76.14%   +0.01%     
==========================================
  Files          34       34              
  Lines       10007    10007              
==========================================
+ Hits         7618     7620       +2     
+ Misses       2389     2387       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@llogick
Copy link
Contributor

llogick commented Jan 5, 2024

Past experience tells me it might be a good idea to rename the current build runner and use it with zig versions < 2030, some/certain projects only update in large increments.

@Techatrix
Copy link
Member

Past experience tells me it might be a good idea to rename the current build runner and use it with zig versions < 2030, some/certain projects only update in large increments.

Any projects that doesn't target the latest Zig shouldn't use the latest ZLS as well.
I would rather have the multi build runner system removed at some point instead of extending it with more versions.
Previous Discussion on this: #1020 (comment)

@llogick
Copy link
Contributor

llogick commented Jan 5, 2024

Past experience tells me it might be a good idea to rename the current build runner and use it with zig versions < 2030, some/certain projects only update in large increments.

Any projects that doesn't target the latest Zig shouldn't use the latest ZLS as well. I would rather have the multi build runner system removed at some point instead of extending it with more versions. Previous Discussion on this: #1020 (comment)

Yeah, well #1396

@Techatrix Techatrix force-pushed the build-system-update branch from e87f7d3 to 8a35990 Compare January 5, 2024 16:05
@Techatrix Techatrix force-pushed the build-system-update branch from 8a35990 to 9f55463 Compare January 5, 2024 16:06
@xdBronch
Copy link
Contributor Author

xdBronch commented Jan 5, 2024

realistically it doesnt matter since the 2 commits are like a day apart but updating the minimum version to the expectEqual change isnt required afaict. i tested earlier and the updated tests work with the new and old expectEqualDeep

@Techatrix
Copy link
Member

FYI, I have edited your PR and done the following:

  • updated the minimum build version again
  • squashed some of your commits
  • updated build.zig.zon so that the commit hashes refer to the master/main branch
  • updated everything nix related

@Techatrix
Copy link
Member

realistically it doesnt matter since the 2 commits are like a day apart but updating the minimum version to the expectEqual change isnt required afaict. i tested earlier and the updated tests work with the new and old expectEqualDeep

The minimum build version had to be updated anyway because of ziglang/#18438, so I did it just in case.

@xdBronch
Copy link
Contributor Author

xdBronch commented Jan 5, 2024

ah i actually did something a little sneaky before 😄
i made the minimum version refer to the commit that fixed that issue but the comment refer to the big module pr. in hindsight it probably didnt make much sense to do so but i was being overly pedantic, the comment above says document the latest breaking change that caused a change to the string below: but it was a bug fix not a breaking change :p
either way it doesnt matter because now CI is failing because of yet another breaking change haha

@Techatrix Techatrix force-pushed the build-system-update branch from 9f55463 to c7d1b70 Compare January 5, 2024 16:24
@Techatrix Techatrix force-pushed the build-system-update branch from c7d1b70 to 9fbd994 Compare January 5, 2024 16:28
Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

Everything looks to be in order finally.
The only thing missing are the nix flakes which I will update in a follow up PR once https://github.com/mitchellh/zig-overlay has updated again.

@Techatrix Techatrix merged commit 3942971 into zigtools:master Jan 5, 2024
8 checks passed
@xdBronch xdBronch deleted the build-system-update branch January 5, 2024 16:47
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.

3 participants