Skip to content

build: fix build for latest zig version #200

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

Closed
wants to merge 3 commits into from

Conversation

sterchelen
Copy link

@sterchelen sterchelen commented Mar 18, 2023

Currently, you can't build ziglings from zig's master branch (at the time of writing, ziglang/zig@c31007b is master's HEAD)
This patch set adapts ziglings' build code to reflect the changes on latest zig version.


A large and impactful change was made on zig's build facilities. The following PR ziglang/zig#14647 introduced those changes.

To be more precise, here are the impacts

  • the color handling on tty
  • the removal of the LogStep logic

As part of ziglang/zig#14647 and more
specifically this zig commit
ziglang/zig@bf73620,
the color is now communicated via env vars.

Signed-off-by: Nicolas Sterchele <[email protected]>
Step's Options introduced from the following PR
ziglang/zig#14647 and more precisely
this commit ziglang/zig@02381c0

Also, MakeFn now takes a std.Progress.Node
introduces by ziglang/zig@0e07879
@chrboesch
Copy link
Collaborator

Thank you for this hint, but there is something wrong. You have deleted the ziglings logo. 😮

@sterchelen
Copy link
Author

Thank you for this hint, but there is something wrong. You have deleted the ziglings logo. open_mouth

The main reason I've removed it, is that LogStep struct was removed from zig's Build system. In other words, now there is no easy way to show the ziglings header only for the verify_all | named_chain steps.

I see two options:

  1. We show the ziglings header every time we run the zig build command, so no dependOn for the header step.
  2. We create a custom step to show the header as before. Though, personally I think it's too much complexity added for just a header shown on stdout.

Hope I was clear 😁

@chrboesch
Copy link
Collaborator

The problem is bigger and requires a general rebuild of the build system: #202

@sterchelen
Copy link
Author

The problem is bigger and requires a general rebuild of the build system: #202

@chrboesch Happy to participate 👍🏼

@chrboesch
Copy link
Collaborator

@sterchelen You are welcome! 😄

@chrboesch
Copy link
Collaborator

Logo problem solved so far, but the main problem with asynchrounus tasks is still there: #203

@sterchelen
Copy link
Author

@chrboesch I think our build.zig logic (with the new zig version that has the build "parallel" feature) is parallel aware by default as we are already leveraging zig's build step facilities; for each exercise we run b.addExecutable() which allows to naturally build them in parallel.

I tried to run my PR on zig's master branch VS a checkout before the merge of ziglang/zig#14647 ; I confirm that the build is faster (though I don't have yet complete all the exercises, would be cool to see a whole run)

One thing I noticed though is that the new build parallel feature doesn't respect the order of the exercices array.

Finally, we could "cherry-pick" your header step fix from your PR into mine?


side note: I am new to zig, so bear with me if I say/do stupid things 😬

@sterchelen
Copy link
Author

@chrboesch it seems you did the job... closing then.

@sterchelen sterchelen closed this Mar 20, 2023
@chrboesch
Copy link
Collaborator

@chrboesch it seems you did the job... closing then.

No, I didn't. This is only a workaround that Ziglings can used manually. A real solution must work as before. What I did ist some experiments, but there are some bugs in the build system, so I opened an issue yesterday.

The idea for a sustainable solution is to use the arguments in the build to select whether to compile all tasks one by one, and then re-run the build system one step at a time. In principal it works. I tested yesterday.

@chrboesch
Copy link
Collaborator

@sterchelen Let's diskuss further here: #202

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