Add run compiler command #874

tiehuis · 2018-03-30T23:41:53Z

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).

#!/home/me/local/bin/zig run

const std = @import("std");
const os = std.os;

const allocator = std.debug.global_allocator;

pub fn main() void {
    var args_it = os.args();

    while (args_it.next(allocator)) |arg_or_err| {
        std.debug.warn("{}\n", arg_or_err);
    }
}

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.

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 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.

Closes #466.

tiehuis · 2018-03-31T01:10:10Z

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?

andrewrk · 2018-03-31T02:27:59Z

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 zig run we shouldn't support that, since it's really meant to be a convenience thing. If a user needs more complicated compiler invocation commands, they can use the normal compilation process or zig build.

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.

andrewrk · 2018-03-31T04:48:39Z

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 zig-cache/ and delete it before building, even though we don't do any caching yet! Years of programming experience has taught us to never trust caches.

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 zig foo.zig syntax is that not supporting zig compiler arguments would be an easier caching problem.

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.

tiehuis · 2018-03-31T05:30:11Z

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.

andrewrk · 2018-03-31T06:08:59Z

src/os.cpp

    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


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.

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.

tiehuis · 2018-03-31T06:35:31Z

src/os.cpp

@@ -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) {


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.

Sounds good 👍

andrewrk · 2018-03-31T14:49:29Z

src/main.cpp

+                    }
+
+                    Termination term;
+                    os_spawn_process(buf_ptr(run_exec_path), args, &term);


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.

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.

tiehuis force-pushed the run-cmd branch 2 times, most recently from a3792bc to 077414e Compare March 31, 2018 00:25

tiehuis force-pushed the run-cmd branch from 077414e to cb52afc Compare March 31, 2018 06:04

andrewrk reviewed Mar 31, 2018

View reviewed changes

tiehuis force-pushed the run-cmd branch from cb52afc to 2fb9ce7 Compare March 31, 2018 06:33

tiehuis commented Mar 31, 2018

View reviewed changes

andrewrk reviewed Mar 31, 2018

View reviewed changes

andrewrk approved these changes Mar 31, 2018

View reviewed changes

tiehuis merged commit 2e5115b into master Apr 1, 2018

tiehuis deleted the run-cmd branch April 1, 2018 05:03

timotheecour mentioned this pull request Apr 24, 2021

--usenimcache (implied by nim r main) now caches some compile options to avoid recompiling when project was previously compiled with such options. nim-lang/Nim#17829

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add run compiler command #874

Add run compiler command #874

tiehuis commented Mar 30, 2018

tiehuis commented Mar 31, 2018

andrewrk commented Mar 31, 2018

andrewrk commented Mar 31, 2018 •

edited

tiehuis commented Mar 31, 2018

andrewrk Mar 31, 2018 •

edited

tiehuis Mar 31, 2018

tiehuis Mar 31, 2018

andrewrk Mar 31, 2018

andrewrk Mar 31, 2018

tiehuis Apr 1, 2018

Add run compiler command #874

Add run compiler command #874

Conversation

tiehuis commented Mar 30, 2018

tiehuis commented Mar 31, 2018

andrewrk commented Mar 31, 2018

andrewrk commented Mar 31, 2018 • edited

tiehuis commented Mar 31, 2018

andrewrk Mar 31, 2018 • edited

Choose a reason for hiding this comment

tiehuis Mar 31, 2018

Choose a reason for hiding this comment

tiehuis Mar 31, 2018

Choose a reason for hiding this comment

andrewrk Mar 31, 2018

Choose a reason for hiding this comment

andrewrk Mar 31, 2018

Choose a reason for hiding this comment

tiehuis Apr 1, 2018

Choose a reason for hiding this comment

andrewrk commented Mar 31, 2018 •

edited

andrewrk Mar 31, 2018 •

edited