build: fix the build system #212

perillo · 2023-04-02T13:11:07Z

In this branch I plan to simplify, improve and fix the ziglings build system.

Tasks

build.zig: incorrect handling of the ziglings logo #211
build: using zig 0.6.0 will cause a compiler error #210
Tested zig build with 0.6.0 to 0.10.0.
build: incorrect description of the named_install step #213
I forgot to mark build: incorrect description of the named_install step #213 as closed; I will have to update the commit message and force push it.

Note that the comment of making Zig 0.5.0 compatible with the version check by moving the code to a separate file is incorrect, since the compiler will parse all the files.
ATTENTION: Ziglings currently only works manually with the new build system of Zig since 0.11.0-dev.2157 #202
This is the last commit of this branch. There are still several issues I would like to solve, and I will use a different branch.

Currently, the logo is always printed when the build script is executed, resulting in the logo being printed twice with `zig build -h` and `zig build -l`. Make the logo a build step, so that the logo is printed to stderr only when necessary. Closes ratfactor#211

chrboesch · 2023-04-02T19:49:47Z

That's ok, but I will do the automatic tests.

The version check for Zig 0.6.0 was incorrect since commit 971ab7f (Use a zig build script to run ziglings). Move compatibility support to a separate file, in order to simplify build.zig. In case of incompatible version, exit with code 3 instead of 0, in order to detect the case of failure in a test (to be implemented). Remove the use of comptime when checking compatibility at the start of the build function, since it is not necessary. Closes ratfactor#210.

perillo · 2023-04-03T09:48:22Z

That's ok, but I will do the automatic tests.

Good.

I'm planning to add some simple tests, starting with testing that old versions of zig will fail to build zig build with exit code 3.
The tests will be implemented as shell scripts, using sh on Unix systems and cmd.exe on Windows.

Remove the logo step, and use PrintStep for the header step. The logo step was added as a quick fix after the Builder.addLog function was removed. Now the logo is no longer shown when running `zig build -l` or `zig build -h`.

Replace the description of the named_install step from "Install {s} to zig-cache/bin" to "Copy {s} to prefix path". The latter has been adapded from the description of the builtin install step. Ad an empty line before the build_step variable, in order to improve readability. Closes ratfactor#213

perillo · 2023-04-06T13:09:29Z

If you think the code is good and it works correctly, you can merge into main, so that more people can test it.

chrboesch · 2023-04-06T14:40:18Z

I assume it only works right with some of your PRs in Zig, right?

chrboesch · 2023-04-06T14:55:19Z

Another question: To start a exercise specifically, the command e.g. zig build -Dexno=1 must now be entered. This is now a bit more cumbersome than before, where an zig build 1 worked. Do you think you could still change that it works again as before?

perillo · 2023-04-06T14:58:57Z

I assume it only works right with some of your PRs in Zig, right?

It works once the branch in the PR is merged.
The fix is in the latest commit, but you need all the commits.

chrboesch · 2023-04-06T15:01:21Z

I checked out your improve-build branch for test. Did I miss something there?

perillo · 2023-04-06T15:02:29Z

Another question: To start a exercise specifically, the command e.g. zig build -Dexno=1 must now be entered. This is now a bit more cumbersome than before, where an zig build 1 worked. Do you think you could still change that it works again as before?

What I can change is renaming the option to D=1. The previous implementation associated 4 steps to each exercise, and I don't really like it.

chrboesch · 2023-04-06T15:04:40Z

What I can change is renaming the option to D=1. The previous implementation associated 4 steps to each exercise, and I don't really like it.

Yes, I think that would be better (nicer).

perillo · 2023-04-06T15:26:27Z

What I can change is renaming the option to D=1. The previous implementation associated 4 steps to each exercise, and I don't really like it.

Yes, I think that would be better (nicer).

Done.

Another possible solution is to remove the chain, and instead add an executable (the driver) that it is build and run by the Zig build system.

The driver will keep the exercise state on the filesystem.

chrboesch · 2023-04-06T15:32:55Z

You mean the first call of zig build creates the driver program and all further calls then start this program, which processes the individual exercises?

chrboesch · 2023-04-06T15:50:27Z

run zig build

looks for zig-out/bin/ziglings
if not found compile src/ziglings

zig build runs zig-out/bin/ziglings

ziglings looks for zig-out/bin/001_hello
if not found

ziglings runs zig build -Dn=1
...

perillo · 2023-04-06T16:24:23Z

I checked out your improve-build branch for test. Did I miss something there?

What do you mean?

chrboesch · 2023-04-06T17:02:11Z

I checked out your improve-build branch for test. Did I miss something there?

What do you mean?

You write "The default step is zigling (note the singular form)." but when I run zig build zigling there is no step. When I run zig build -h I see a step called ziglings. When I run zig build or zig build ziglings I get the same parallel behavior as before. Therefor I asked if I missed something.

perillo · 2023-04-06T17:28:08Z

I checked out your improve-build branch for test. Did I miss something there?

What do you mean?

You write "The default step is zigling (note the singular form)." but when I run zig build zigling there is no step. When I run zig build -h I see a step called ziglings. When I run zig build or zig build ziglings I get the same parallel behavior as before. Therefor I asked if I missed something.

I checked again more carefully, and it is indeed a bug, sorry. I will try to fix it.

The new parallel build support in Zig broke the exercise chain, so that each esercise check is no longer strictly serialized. 1. Add the Dexno option, in order to isolate the chain starting from a named exercise from the normal chain, thus simplify the code. The current code have an additional issue: it added 4 x n steps, making reading the help message or the list of steps very hard. Add only the `install`, `uninstall`, `zigling`, `test` and `start` steps. The last three steps match the old steps `n`, `n_test` and `n_start`. The default step is zigling (note the singular form). The `install` step override the builtin install step, showing a custom description and matches the old `n_install` step. The uninstall step was added for consistency, so that the description is consistent. Setup a new chain starting at `zig build -Dexno=n start` so that it is stricly serialized. The behavior should be the same as the old one. 2. Handle the code for all the exercises separately. Add only the `ziglings step`, making it the default step, in addition to the install and uninstall steps. Setup a new chain starting at the first exercise, to that it is strictly serialized. The behavior should be the same as the old one. The current code has a know issue: the messages from the ZiglingStep and the ones from the compiler compilation progress are interleaved, but each message is written atomically, due to the use of `std.debug.getStderrMutex()`. Update the README.md file. Closes ratfactor#202

perillo · 2023-04-07T09:39:56Z

I removed all the unnecessary no-op custom steps that where present in the previous code.

During my tests, the behavior seems correct, but, again, this needs more people to check the code.

perillo · 2023-04-07T13:52:14Z

I noted now that the last commit in this PR broke the compatibility support for old Zig compilers.

The problem is the use of the new for loop syntax, that is causing the grammar parsing step to fail.

Fortunately there is a simple solution, without the need to use a variable for the loop index. I will apply after the branch has been merged.

This is primarily to make users aware that there has been a change in the call for individual exercises.

chrboesch · 2023-04-07T16:26:07Z

@perillo Thank you very much for your great effort!

perillo · 2023-04-07T16:36:58Z

@perillo Thank you very much for your great effort!

Thanks to you for noticing that I forgot to update the message.

However, was updating the needed version really necessary?

chrboesch · 2023-04-07T16:43:08Z

However, was updating the needed version really necessary?

Yes I think so, because the users have now all started with zig build n the exercises, which now no longer works. Also, now zig build works again. I think this way it will be clearer to them.

BTW, in the README there are the following additional hints: Advanced usage
Do you think we can implement this with standard build paramteres like zig build -Dtest=1 etc.?

perillo · 2023-04-07T18:42:55Z

However, was updating the needed version really necessary?

Yes I think so, because the users have now all started with zig build n the exercises, which now no longer works. Also, now zig build works again. I think this way it will be clearer to them.

BTW, in the README there are the following additional hints: Advanced usage Do you think we can implement this with standard build paramteres like zig build -Dtest=1 etc.?

They are already available:

$ zig build -Dn=1 -l

  install                      Install 001_hello.zig to prefix path
  uninstall                    Uninstall 001_hello.zig from prefix path
  test                         Run 001_hello.zig without checking output
  zigling (default)            Check the solution of 001_hello.zig
  start                        Check all solutions starting at 001_hello.zig

chrboesch · 2023-04-08T11:04:12Z

Ok, I have not tested that. Then I just have to rewrite the README.

build: make the logo a build step

perillo mentioned this pull request Apr 4, 2023

ATTENTION: Ziglings currently only works manually with the new build system of Zig since 0.11.0-dev.2157 #202

Closed

perillo added 2 commits April 4, 2023 18:32

build: replace the logo step with the header step

9354de0

Remove the logo step, and use PrintStep for the header step. The logo step was added as a quick fix after the Builder.addLog function was removed. Now the logo is no longer shown when running `zig build -l` or `zig build -h`.

perillo force-pushed the improve-build branch from 77436a1 to 7b793d8 Compare April 5, 2023 19:20

perillo force-pushed the improve-build branch from 1a7d636 to 6e2f174 Compare April 6, 2023 15:23

perillo force-pushed the improve-build branch from 6e2f174 to 0d56ba3 Compare April 7, 2023 09:36

chrboesch added 2 commits April 7, 2023 18:06

help text for parameters adapted

f4408f9

Insert current zig version for compatiblity!

45ccfa7

This is primarily to make users aware that there has been a change in the call for individual exercises.

chrboesch merged commit 0f54e2e into ratfactor:main Apr 7, 2023

chrboesch mentioned this pull request Apr 8, 2023

Adjustment of the 'Advanced usage' in the README #223

Closed

perillo changed the title ~~build: make the logo a build step~~ build: fix the build system Apr 12, 2023

perillo mentioned this pull request Apr 12, 2023

Update the code to the new zig version #235

Merged

vamega pushed a commit to vamega/ziglings that referenced this pull request Jul 25, 2023

Merge pull request ratfactor#212 from perillo/improve-build

3685fc7

build: make the logo a build step

fleimgruber pushed a commit to fleimgruber/ziglings that referenced this pull request Oct 26, 2023

Merge pull request ratfactor#212 from perillo/improve-build

e242d82

build: make the logo a build step

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: fix the build system #212

build: fix the build system #212

perillo commented Apr 2, 2023 •

edited

Loading

chrboesch commented Apr 2, 2023

perillo commented Apr 3, 2023

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

chrboesch commented Apr 6, 2023

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

chrboesch commented Apr 6, 2023 •

edited

Loading

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

perillo commented Apr 6, 2023

perillo commented Apr 7, 2023

perillo commented Apr 7, 2023

chrboesch commented Apr 7, 2023

perillo commented Apr 7, 2023

chrboesch commented Apr 7, 2023

perillo commented Apr 7, 2023

chrboesch commented Apr 8, 2023

build: fix the build system #212

build: fix the build system #212

Conversation

perillo commented Apr 2, 2023 • edited Loading

Tasks

chrboesch commented Apr 2, 2023

perillo commented Apr 3, 2023

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

chrboesch commented Apr 6, 2023

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

chrboesch commented Apr 6, 2023 • edited Loading

perillo commented Apr 6, 2023

chrboesch commented Apr 6, 2023

perillo commented Apr 6, 2023

perillo commented Apr 7, 2023

perillo commented Apr 7, 2023

chrboesch commented Apr 7, 2023

perillo commented Apr 7, 2023

chrboesch commented Apr 7, 2023

perillo commented Apr 7, 2023

chrboesch commented Apr 8, 2023

perillo commented Apr 2, 2023 •

edited

Loading

chrboesch commented Apr 6, 2023 •

edited

Loading