-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add run compiler command #874
Conversation
a3792bc
to
077414e
Compare
There would be an issue if you wanted to pass flags to the program. Right now we allow flags in any position, even after a positional argument. We would need to disallow flags after positionals for this case so we could treat all trailing arguments, positional or flags, as runtime arguments. I think being able to add flags after a command is still too useful. For example, if you have forgotten one or just want to a previous command from your history with an added flag. Unless, you mean when omitting the run command entirely all arguments are treated as runtime arguments, with the first being the input file? |
Yep that's what I meant. The only problems I can foresee are things like, wanting to pass include directories or link libraries. But maybe for I do think that it would make sense to be able to depend on external zig packages though for a run script. We'll have to revisit that idea once we have a package manager. |
Normally I'm in favor of incremental improvements - but there's one thing where I think we should have a 100% correctly working implementation, or not at all - and that is for caching. I've already noticed that people don't trust Let's enumerate an exhaustive list of things that need to go into the cache hash. I'll check the ones that it looks like you handled in this PR:
That's all I can think of. Can you think of any other ways to fool the cache? I also think we should use a more robust hash such as sha-256 since we're using it to detect collisions, and so we don't want something with the possibility of collisions. I think that could even be a security issue in some cases. One other reason to use the less powerful It's a bit of work to have a foolproof cache. It's why I've been putting off this issue and the other caching issues. But I really think we should not merge a cache that is not perfect into master. To be clear, I'm happy to add this feature with no caching at all. |
Fair points. That is a good list and highlights the many different things we need to consider. Since the scope of this is much larger, how about I remove the caching for this current PR and we think about it further and implement a robust version with caching in the self-hosted version? I'll still use a global cache for run, but it'll be removed/replaced on every invocation as is. Sound good? I can make a new issue and put these points in and we can get some discussion going there. |
src/os.cpp
Outdated
for (;;) { | ||
size_t amt_read = fread(buf_ptr(out_buf) + actual_buf_len, 1, buf_size, f); | ||
actual_buf_len += amt_read; | ||
|
||
#if defined(ZIG_OS_POSIX) | ||
// posix shebang for zig run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os_fetch_file
is intended to return file contents verbatim. I think it would be better to put this code outside os.cpp. This could break @embedFile
or the code that reads a linker script, for example.
Even on non-posix OSes we want to allow shebang lines, right? It seems like the same file should work if you do zig run
on windows, or on linux. Same as how it works with Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
'zig run file.zig' builds a file and stores the artifacts in the global cache. On successful compilation the binary is executed. 'zig run file.zig -- a b c' does the same, but passes the arguments a, b and c as runtime arguments to the program. Everything after an '--' are treated as runtime arguments. On a posix system, a shebang can be used to run a zig file directly. An example shebang would be '#!/usr/bin/zig run'. You may not be able pass extra compile arguments currently as part of the shebang. Linux for example treats all arguments after the first as a single argument which will result in an 'invalid command'. Currently there is no customisability for the cache path as a compile argument. For a posix system you can use `TMPDIR=. zig run file.zig` to override, in this case using the current directory for the run cache. The input file is always recompiled, even if it has changed. This is intended to be cached but further discussion/thought needs to go into this. Closes #466.
@@ -291,13 +291,39 @@ void os_path_resolve(Buf *ref_path, Buf *target_path, Buf *out_abs_path) { | |||
return; | |||
} | |||
|
|||
int os_fetch_file(FILE *f, Buf *out_buf) { | |||
int os_fetch_file(FILE *f, Buf *out_buf, bool skip_shebang) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a look and to me it still seemed appropriate keep the file reading in os.cpp
. I've just opted for a variable to specify if we should skip the shebang and adjusted uses accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍
} | ||
|
||
Termination term; | ||
os_spawn_process(buf_ptr(run_exec_path), args, &term); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a good first step, and I'm happy to merge with it like this, but 2 questions:
- Do you think we should use execve? Completely replace the current process with the new one?
- If we did this, what about Windows?
I'm thinking about the case where the process we're running gets SIGABRT. With this code, zig would call exit 1. Still exiting with an error, but a different way. Maybe it's fine though. I think if you usd gdb with follow-fork-child, it would still catch the abort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll need to look more into the small details to see if there is a way we could manage that use case. Will leave for now.
See #466.
This is fairly simplistic and doesn't implement all the ideas from the proposal but I think it'll cover the vast majority of use cases right now. Summary of the features below.
A little messy, I have some visions for how I'd like to see the compiler cli but that's another issue. Also untested on Windows but provided I implemented the os details without compiler errors they should do the job.
I've skimped on tests for the moment.
Here is a simple test program that can be used to verify arguments (modify the shebang location to suit).
zig run file.zig
builds a file and stores the artifacts in the globalcache. On successful compilation the binary is executed.
zig run file.zig -- a b c
does the same, but passes the arguments a,b and c as runtime arguments to the program. Everything after an
--
aretreated as runtime arguments.
If the file was not modified or renamed and the compile arguments have not
changed (order cannot change either) then a cached version may be used
instead of recompiling. Runtime arguments do not trigger a rebuild.
On a posix system, a shebang can be used to run a zig file directly. An
example shebang would be
#!/usr/bin/zig run
. You may not be able passextra compile arguments currently as part of the shebang. Linux for example
treats all arguments after the first as a single argument which will result
in an 'invalid command'.
Currently there is no customisability for the cache path as a compile
argument. For a posix system you can use
TMPDIR=. zig run file.zig
tooverride, in this case using the current directory for the run cache.
Closes #466.