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

Discrepancy between @src() when invoked via zig build or in test and project code #19570

Closed
Tracked by #8
swan-www opened this issue Apr 7, 2024 · 16 comments
Closed
Tracked by #8
Labels
bug Observed behavior contradicts documented or intended behavior
Milestone

Comments

@swan-www
Copy link

swan-www commented Apr 7, 2024

Zig Version

0.12.0-dev.3522+b88ae8dbd

Steps to Reproduce and Observed Behavior

Tested on Windows, I've not tested on other platforms.

When the @src() builtin is called from build.zig (or any code that runs during zig build), in the returned SourceLocation the .file field appears to be always an absolute path, e.g:
D:\devspace\myproject\build.zig

But when called in project code, it only returns the name of the file and not a full path:
projectcode.zig

The case where I hit this was writing a test for some of my build utils, inside build.zig. The functionality works when calling zig build but if I call zig test build.zig then the different value returned by @src() causes my tests to fail.

Expected Behavior

I expect the values returned by @src() in build.zig and when running zig test build.zig to be consistent with each other.

@swan-www swan-www added the bug Observed behavior contradicts documented or intended behavior label Apr 7, 2024
@swan-www
Copy link
Author

swan-www commented Apr 7, 2024

I think personally my preference would be that @src() always gives you a SourceLocation with an absolute path to the file -- I think this is more useful as it provides more information, and I think it's guaranteed to give a unique path for each source file. Whereas if it only returns the filename, it's fairly plausible the developer may have two files somewhere in the project that have the same name, leading to ambiguity.

@RossComputerGuy
Copy link
Contributor

I agree, I think absolute paths are more distinct. If you have some weird zig project that copies or adds things to the cache and expose as a module and you have similar paths in a different module then it could be difficult to determine where the problems are at.

@rohlem
Copy link
Contributor

rohlem commented Apr 7, 2024

preferring absolute paths

If I understand correctly, these paths are baked into the binary.
I would prefer if Zig artifacts had support for reproducibility (to the extent technically possible).
Changing under which path (and on which system) I build a project should (to the extent possible) not lead to changes in the produced artifacts.
Therefore I think I would prefer some form of relative path... though I assume they will need to support
multiple starting points for Zig's global cache, the project's build cache, and one for the project root module.
(I think other modules are always in one of the cache directories, if not then they need one as well.)
So that would make them pseudo-absolute, starting from a virtual/symbolic root.

@swan-www
Copy link
Author

swan-www commented Apr 7, 2024

@rohlem I'd be fine with that too, so long as there's a builtin for getting the cache path(s) too, so that the developer can construct an absolute path to the source file if they deem it necessary. Otherwise you have a relative path, but as far as I can tell, no method for getting the base path that it is relative to.

@castholm
Copy link
Contributor

castholm commented Apr 7, 2024

I will second the notion that paths returned by @src should not be absolute, for reproducibility and privacy reasons (absolute paths can leak information about the host system).

@swan-www
Copy link
Author

swan-www commented Apr 7, 2024

Ignore my initial comment, I agree that relative paths should be the preference.

I'm trying to think about this from the angle that you might want to use src paths with some external tooling during development, and the external tooling won't necessarily know how to resolve the relative paths if there's no convention or method for determining the base path from which those relative paths were taken.

I guess the hardline of this will probably be that tooling shouldn't shape the language, the language should shape the tooling. So maybe the expectation is that the base path needs to be manually specified by the developer, to the external tooling. And that the base path should exist outside of any compiled zig binaries (even debug ones).

I'm interested to hear what others think about the above situation/what the expectation should be.

@mnemnion
Copy link

mnemnion commented Aug 4, 2024

I've run into a related problem: In Zig 0.13.0 a file at ./src/file.zig gets the SourceLocation path src/file.zig, but on the master branch, the path is just file.zig. This is a problem, because Zig files can be in the working directory, not just in ./src, or anywhere, really. So ohsnap doesn't get the information it needs to find the file from @src() anymore, and that's kind of what @src is for, right? As a user-facing feature at least.

Perhaps the SourceLocation struct can carry a separate field for the file path and the file name? It's a regression in capability to be unable to combine fs.cwd() with a SourceLocation and find the file. I assume there's some reasoning behind the change, but it would be nice if that reasoning didn't make @src() useless, which it now is.

@rohlem
Copy link
Contributor

rohlem commented Aug 5, 2024

@mnemnion From quick testing it seems like currently Zig strips only the path prefix of the root source file.
So zig test src/root.zig would strip that file to root.zig, but src/a/b/x.zig still receives .file = "a/b/x.zig".
As long as you know the path to the root source file, you should be able to use that as the base for all such relative file paths.

zig test internally produces an executable that is then run. From Zig's point of view, there is no logical dependency from this executable on any the source files it was generated from afterwards - those could lie on a different machine, or have been changed/deleted/moved in the meantime.
From this perspective, I think it makes sense that the user's execution setup needs to separately know where the root source file that was used was/is located.
I think your ohsnap module could f.e. provide its own utility function for use in build.zig that takes the file path and passes it along as cwd of the tests' run step. (would then return a struct with both the *std.Build.Step.Compile and the *std.Build.Step.Run)
Afaiu that should solve your use case.

@mnemnion
Copy link

mnemnion commented Aug 5, 2024

This doesn't work, unfortunately, because ./file.zig and ./src/file.zig can't be distinguished.

It's true that a heuristic will handle this in most cases, but a library which alters someone else's source code should be more careful than that. Thanks for the suggestion, I've commented on the issue tracking this, which is a better place for a detailed discussion.

@rohlem
Copy link
Contributor

rohlem commented Aug 5, 2024

This doesn't work, unfortunately, because ./file.zig and ./src/file.zig can't be distinguished.

@mnemnion Please re-read my message. The path prefix of the compilation's (EDIT:module's) root file is stripped from the @src().file paths of all files in that module.
If the root file is located in a folder ./src, then it will be missing. If the root file is located in . and the rest of the files are in a folder src, those other files' @src() will have that path prefix in their .file field.
I just re-tested this to confirm. Feel free to describe a scenario where the behavior of your testing differs.
Note that the root file is whatever you specify to zig run/zig test / in std.Build.addTest etc. , NOT the build.zig file.

@mnemnion
Copy link

mnemnion commented Aug 6, 2024

I've explained the several scenarios where this doesn't work in #20864 already, as well as a solution which will include both a cwd-relative and a relocatable file descriptor in SourceLocation.

@mnemnion
Copy link

mnemnion commented Aug 6, 2024

If you can show me working code which will allow ohsnap to open a file given only the information now included in @src(), I would be happy to test if it actually works for my purposes. Maintaining a version-specific variation in how the library does that for awhile is not a big deal at all.

@rohlem
Copy link
Contributor

rohlem commented Aug 6, 2024

I've explained the several scenarios where this doesn't work in #20864 (comment) already

@mnemnion Your comment there doesn't reference my reply here, where I outline that the missing path prefix should be exactly the path prefix of the module's root source file.
The solution is to change the cwd to the directory of the source file (or alternatively to prefix the paths from @src().file with exactly that path prefix, but that seems more brittle to me).

as well as a solution which will include both a cwd-relative and a relocatable file descriptor in SourceLocation.

I'm not responsible for the relevant change, but to me it seems like the intention behind the change was to make a compilation command zig <command> <source file> no longer depend on the parent path of the source file.
Your proposed resolution would allow users to reintroduce this dependency. It's up to whoever made the decision, but I think for the same reason the .file field was changed (presumably ensuring reproducible builds regardless of the cwd of the zig invocation), they might not want to reintroduce another field .file_path_from_compiler_invocation_cwd that compromises reproducibility the same way the old behavior did.

If you can show me working code which will allow ohsnap to open a file given only the information now included in @src()

I've already outlined a solution above, here is a full example `build.zig` project implementing that solution:

//! build.zig
const std = @import("std");

pub fn build(b: *std.Build) void {
    const target = b.standardTargetOptions(.{});
    const optimize = b.standardOptimizeOption(.{});

    const lib_unit_tests = addTestAndRunInSameDirectory(b, b.path("src/root.zig"), target, optimize);

    const test_step = b.step("test", "Run unit tests");
    test_step.dependOn(&lib_unit_tests.run_step.step);
}

pub const ArtifactAndRunStep = struct {
    artifact: *std.Build.Step.Compile,
    run_step: *std.Build.Step.Run,
};
pub fn addTestAndRunInSameDirectory(
    b: *std.Build,
    root_source_file_path: std.Build.LazyPath,
    target: std.Build.ResolvedTarget,
    optimize: std.builtin.OptimizeMode,
) ArtifactAndRunStep {
    const artifact = b.addTest(.{
        .root_source_file = root_source_file_path,
        .target = target,
        .optimize = optimize,
    });
    const run_step = b.addRunArtifact(artifact);
    run_step.setCwd(root_source_file_path.dirname()); //run the test executable in the directory of the root source file
    return .{
        .artifact = artifact,
        .run_step = run_step,
    };
}
//! src/root.zig
const std = @import("std");
const testing = std.testing;

test "reading and printing this source file" {
    const alloc = testing.allocator;
    const contents = try std.fs.cwd().readFileAlloc(alloc, @src().file, 65526);
    defer alloc.free(contents);
    std.debug.print("{s}", .{contents});
}

output from invoking zig build test (tested with 0.14.0-dev.617+208baa37c):

test
└─ run test stderr
//! src/root.zig
const std = @import("std");
const testing = std.testing;

test "reading and printing this source file" {
    const alloc = testing.allocator;
    const contents = try std.fs.cwd().readFileAlloc(alloc, @src().file, 65526);
    defer alloc.free(contents);
    std.debug.print("{s}", .{contents});
}

This is just a simple setup without any external modules, but from how I understand the mechanisms behind build.zig and modules, it should work just as well for more complex projects.
Feel free to let me know if something does break.

For direct command line usage of zig test <file> without zig build, I think the only solution currently is to cd into the directory containing the root source file (just like we're doing for the run step in the build.zig in the example provided above).
You could write/provide a wrapper program zigtest which spawns zig test as a subprocess in that file's directory, that would give you the desired behavior.
Or maybe you want to propose a flag for zig test to automatically do this. Maybe that could/should even be the default behavior. I personally wouldn't be affected by such a change, so I haven't considered arguments for/against it yet.

@mnemnion
Copy link

mnemnion commented Aug 6, 2024

ohsnap is a drop-in library which is easy to use.

This would impose a substantial and complex build step on any user of the library. It isn't an acceptable workaround.

I'm not responsible for the relevant change, but to me it seems like the intention

It's an interesting speculation, I suppose.

@rohlem
Copy link
Contributor

rohlem commented Aug 6, 2024

This would impose a substantial and complex build step on any user of the library.

ohsnap.addTestAndRunInSameDirectory(...) isn't much longer than b.addTest(...) followed by b.addRunArtifact(...).

If you want standard zig test to work well with your use case, I implore you to open a proposal:
zig test should run the generated executable in the directory of the provided root source file. Either by default, or when given a CLI flag.
(I assume it would only be consistent to also apply such a change to the corresponding build.zig run step, so that fixes both usage scenarios for you.)
Would that be an acceptable solution for you?

@andrewrk
Copy link
Member

This issue is solved now that @src() returns paths relative to the module root always. The other issue being discussed here is tracked by #20999.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior
Projects
None yet
Development

No branches or pull requests

6 participants