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

build: fix the build system #212

Merged
merged 7 commits into from
Apr 7, 2023
Merged

Conversation

perillo
Copy link
Contributor

@perillo perillo commented Apr 2, 2023

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

Tasks

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
Copy link
Collaborator

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
Copy link
Contributor Author

perillo commented Apr 3, 2023

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
Copy link
Contributor Author

perillo commented Apr 6, 2023

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

@chrboesch
Copy link
Collaborator

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

@chrboesch
Copy link
Collaborator

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
Copy link
Contributor Author

perillo commented Apr 6, 2023

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
Copy link
Collaborator

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

@perillo
Copy link
Contributor Author

perillo commented Apr 6, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

perillo commented Apr 6, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

chrboesch commented Apr 6, 2023

  1. run zig build
  • looks for zig-out/bin/ziglings
  • if not found compile src/ziglings
  1. zig build runs zig-out/bin/ziglings
  • ziglings looks for zig-out/bin/001_hello
  • if not found
  1. ziglings runs zig build -Dn=1
  2. ...

@perillo
Copy link
Contributor Author

perillo commented Apr 6, 2023

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

What do you mean?

@chrboesch
Copy link
Collaborator

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
Copy link
Contributor Author

perillo commented Apr 6, 2023

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
Copy link
Contributor Author

perillo commented Apr 7, 2023

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
Copy link
Contributor Author

perillo commented Apr 7, 2023

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 chrboesch merged commit 0f54e2e into ratfactor:main Apr 7, 2023
@chrboesch
Copy link
Collaborator

@perillo Thank you very much for your great effort!

@perillo
Copy link
Contributor Author

perillo commented Apr 7, 2023

@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
Copy link
Collaborator

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
Copy link
Contributor Author

perillo commented Apr 7, 2023

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
Copy link
Collaborator

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

@perillo perillo changed the title build: make the logo a build step build: fix the build system Apr 12, 2023
vamega pushed a commit to vamega/ziglings that referenced this pull request Jul 25, 2023
fleimgruber pushed a commit to fleimgruber/ziglings that referenced this pull request Oct 26, 2023
build: make the logo a build step
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