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

@src().file no longer includes directory #20864

Closed
scheibo opened this issue Jul 29, 2024 · 10 comments
Closed

@src().file no longer includes directory #20864

scheibo opened this issue Jul 29, 2024 · 10 comments
Labels
question No questions on the issue tracker, please.

Comments

@scheibo
Copy link
Contributor

scheibo commented Jul 29, 2024

Zig Version

0.14.0-dev.658+4a77c7f25

Steps to Reproduce and Observed Behavior

Sometime between 0.14.0-dev.544+7aaebd177 and 0.14.0-dev.611+f2bf6c1b1 this changed

$ cat src/main.zig
const std = @import("std");

pub fn main() !void {
    std.debug.print("{s}\n", .{@src().file});
}
$ zig run src/main.zig
main.zig

Not sure if this is an intentional breaking change or not

Expected Behavior

$ zig run src/main.zig
src/main.zig
@scheibo scheibo added the bug Observed behavior contradicts documented or intended behavior label Jul 29, 2024
@mlugg
Copy link
Member

mlugg commented Jul 29, 2024

Intentional change to avoid different output based on weird things like the compiler's cwd. That said, if you have a use case which is impacted, please explain in a comment and we can figure out how to address it.

@mlugg mlugg closed this as not planned Won't fix, can't repro, duplicate, stale Jul 29, 2024
@mnemnion
Copy link

mnemnion commented Aug 4, 2024

$ echo 'const std = @import("std");

   test "where am I" {
       const loc = @src();
       std.debug.print("location: {s}\\n", .{loc.file});
   }
   ' > file.zig

$ zig test file.zig

$ echo 'const std = @import("std");

   test "where am I" {
       const loc = @src();
       std.debug.print("location: {s}\\n", .{loc.file});
   }
   ' > src/file.zig

$ zig test src/file.zig

These produce identical output.

@mnemnion
Copy link

mnemnion commented Aug 4, 2024

This breaks the ohsnap module, which relies on being able to find a file in order to update the snapshot.

It will also break the TigerBeetle snapshot testing which it is directly based upon.

A reasonable solution would be to include the relative-path part as its own field of the SourceLocation struct. Having to manage two versions of the code for code finding would not be difficult.

@rohlem

This comment was marked as outdated.

@rohlem
Copy link
Contributor

rohlem commented Aug 5, 2024

@mnemnion Within a single compilation, files are disambiguated.

If you have a file root.zig:

comptime {
  _ = @import("a/f.zig");
  _ = @import("b/f.zig");
}

and a/f.zig and b/f.zig each have test {@import("std").log.err("{s}", .{@src().file});},
you'll see that with zig test root.zig each test prints the file's relative path from the root.zig file.
Since this is also a quick way to rerun all tests, that seems like a reasonable setup in a build.zig as well, as opposed to adding each file separately.

Alternatively you could navigate into the file's directory - that way the file should always be accessible without any path prefix.

@mnemnion
Copy link

mnemnion commented Aug 5, 2024

If you run the shell commands I sketched out above, you'll see that the current .file field doesn't disambiguate between ./ and ./src/. That's an issue for my library, more Zig programs than you might think put the main logic in the root directory of the repository.

Also, in the 0.13 implementation, this works:

const file_as_slice = try std.fs.cwd().readAlloc(alloc, source_location.file, 65526);

Which would be quite a bit more involved in a situation where the user code needs to guess which directory the file actually lives in.

I think the change is reasonable for what the build system is doing, I'm sure it wasn't done capriciously, but in the service of the task at hand. I see the point in making the .file field relocatable.

But @src() as a user-facing function has its own utility, and both cases should be supported. It's not that this change has made things more difficult, it's made them impossible, because you can also do this:

$ echo 'const std = @import("std");

   test "where am I" {
       const loc = @src();
       std.debug.print("location: {s}\\n", .{loc.file});
   }
   ' > test/file.zig

What does this print? file.zig. There is no programmatic way to resolve these ambiguities, and there is nothing whatsoever stopping someone from having a src/module.zig and a test/module.zig in the same project. In fact, in other ecosystems, that's absolutely commonplace, and someone who is accustomed to that could easily set it up for themselves that way.

A reasonable fix, since the path string is going to be in .rodata anyway, would be to have the .file string work the way it does now in master, but add a field .file_path which works the way .file worked until just recently.

These share a common suffix, so they can be two slices into the same string. That way, the build system gets a relocatable path, without having to do any fancy path handling, and users of @src() can still call try std.fs.cwd().readAlloc(alloc, source_location.file_path, 65526) or what have you, and that will still work.

It's one more field of usize * 2, and it means that @src() stays useful to users, and is a better tool for the build system as well. This seems like an optimal outcome.

@mnemnion
Copy link

mnemnion commented Aug 6, 2024

That said, if you have a use case which is impacted, please explain in a comment and we can figure out how to address it.

@mlugg I've done so, I'm not sure if you see comments in closed issues or not, which is why I'm tagging you here. It would be good to see a final form for this change which continues to support this use case for @src().

@mnemnion
Copy link

mnemnion commented Aug 6, 2024

Having two versions of the .file field doesn't seem like an ideal solution here either.

What about adding a .working_directory to "builtin"? That would probably be more robust anyway. And it doesn't require an extra field on every SourceLocation object, I don't know how many of those are generated on a typical build, it could range from one for every message which needs it, to a great many indeed. There doesn't appear to be anything of that nature in "builtin" currently, at least as shown by zig test src/something.zig --show-builtin.

The outcome I care about is having something I can combine with the SourceLocation in order to be able to open the file. It occurs to me that the change in the .file field might imply that std.fs().cwd() might no longer be something which can consistently be combined with a SourceLocation regardless of how the .file field is generated. It certainly works now, and I hope that some way to accomplish that is worked out here. @src() isn't very useful if it can't open the file in question.

@andrewrk andrewrk added question No questions on the issue tracker, please. and removed bug Observed behavior contradicts documented or intended behavior labels Aug 7, 2024
@andrewrk
Copy link
Member

andrewrk commented Aug 7, 2024

Note that in this example, src/main.zig is the root source file, making src/ the root directory of the module. The expression @src().file from inside src/foo/bar.zig will return "foo/bar.zig".

I think this use case will be solved by adding a field for the module. It still won't show the cwd but it will provide the uniqueness property that is needed. Modules have canonical names that are decided by the CLI invocation.

@mnemnion
Copy link

mnemnion commented Aug 7, 2024

Great, any solution which allows code to go from @src() to opening the associated file is fine by me. I'll keep an eye out for changes which enable this, and test them on the library ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question No questions on the issue tracker, please.
Projects
None yet
Development

No branches or pull requests

5 participants