Skip to content

modules that depend on themselves cannot be tested #14708

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

Closed
leecannon opened this issue Feb 22, 2023 · 10 comments
Closed

modules that depend on themselves cannot be tested #14708

leecannon opened this issue Feb 22, 2023 · 10 comments
Labels
zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@leecannon
Copy link
Contributor

Zig Version

0.11.0-dev.1782+b52be973d

Steps to Reproduce and Observed Behavior

fn createModule(b: *std.Build) *std.Build.Module {
    const recursive_module = b.createModule(
        .{ .source_file = .{ .path = "module/module.zig" } },
    );

    return b.createModule(.{
        .source_file = .{ .path = "module/module.zig" },
        .dependencies = &.{
            .{ .name = "module", .module = recursive_module },
        },
    });
}

using a function like the above to create a module that can import its own root file worked on version 0.11.0-dev.1711+dc1f50e50 but now results in errors like:

$ zig build run
module/module.zig:1:1: error: file exists in multiple modules
module/module.zig:1:1: note: root of module root.module.module
module/module.zig:1:1: note: root of module root.module

Expected Behavior

It should be possible for a module to import its own root file whether that is supporting this hack that I have been using or something akin to @import("root") like @import("module_root")

This allows modules to have deeply nested structures but not have to use the fragile @import("../../../module_root.zig").thing.i.want

@leecannon leecannon added the bug Observed behavior contradicts documented or intended behavior label Feb 22, 2023
@travisstaloch
Copy link
Contributor

I ran into the same thing just now in travisstaloch/protobuf-zig

$ zig build
src/lib.zig:1:1: error: file exists in multiple modules
src/lib.zig:1:1: note: root of module root.protobuf.protobuf
src/lib.zig:1:1: note: root of module root.protobuf

@davidgmbb
Copy link
Contributor

You need to set it after creation, like in here: https://github.com/davidgm94/rise/blob/2dcaf3644b2188c0aa934a2ecac80c8bb6965528/build.zig#L294

@leecannon
Copy link
Contributor Author

leecannon commented Feb 22, 2023

@davidgm94 that works thanks!

Although i'm still wondering whether something like @import("module_root") is warranted as this still feels like a hack that could be broken at any moment.

In the std.Build.Pkg days I use to do something similar where one of the dependencies would be undefined then after construction the undefined dependency would be set to the Pkg itself.

working code:

fn createModule(b: *std.Build) !*std.Build.Module {
    const module = b.createModule(.{
        .source_file = .{ .path = "module/module.zig" },
    });

    try module.dependencies.put("module", module);

    return module;
}

@andrewrk andrewrk added the zig build system std.Build, the build runner, `zig build` subcommand, package management label Feb 22, 2023
@andrewrk andrewrk added this to the 0.11.0 milestone Feb 22, 2023
@andrewrk andrewrk removed the bug Observed behavior contradicts documented or intended behavior label Feb 22, 2023
@travisstaloch
Copy link
Contributor

worked for me too. thanks @davidgm94 💯

if anyone else runs into this issue, here is what i did to get my project compiling again:

ndex 33708f4..7a3cfc9 100644
--- a/build.zig
+++ b/build.zig
@@ -30,6 +30,7 @@ pub fn build(b: *std.build.Builder) !void {
     const protobuf_mod = b.createModule(.{
         .source_file = .{ .path = "src/lib.zig" },
     });
+    try protobuf_mod.dependencies.put("protobuf", protobuf_mod);
 
     const build_options = b.addOptions();
     build_options.addOption(std.log.Level, "log_level", log_level);
@@ -54,10 +55,7 @@ pub fn build(b: *std.build.Builder) !void {
     });
     protoc_zig.install();
     protoc_zig.addOptions("build_options", build_options);
-    protoc_zig.addAnonymousModule("protobuf", .{
-        .source_file = .{ .path = "src/lib.zig" },
-        .dependencies = &.{.{ .name = "protobuf", .module = protobuf_mod }},
-    });
+    protoc_zig.addModule("protobuf", protobuf_mod);

@mlugg
Copy link
Member

mlugg commented Feb 23, 2023

For a bit of context here: this was caused by #14664. Contrary to what it looks like, this actually made module dependency loops possible!

Previously, these only sort of worked, due to a convenient bug in the compiler's module system. The CLI didn't actually have a way to specify recursive dependencies, but if you did a sufficient amount of nesting (generally 1 or 2 levels), it kinda worked anyway. But this was very fragile, and exposed inconsistent behaviour and compiler bugs. The new behaviour has proper, intentional support for dependency loops and anything else you can think of.

The std.Build API can be a bit unintuitive at the minute with these changes. I'm going to propose some changes in today's stage2 meeting to make it neater to use. The TL;DR of the issue here is that you need to make sure you only have one *Module per module: in the original code example here, you have two, which results in this being passed to the CLI as two genuinely distinct modules, leading to an error because then the root file exists in two different modules. Unfortunately, this currently means you have to directly modify mod.dependencies to create dependency loops.

Here's an overview of the updated module behaviour - this should probably be documented properly somewhere else, I'll look at doing that.

  • modules are specified on the CLI with the syntax --mod name:deps:file, where deps is dep,dep,... and dep is [import name=]name
  • the modules depended on by the root package are specified on the CLI through --deps (subject to change, come to today's stage2 meeting!)
  • modules may depend on themselves, directly or indirectly; definition order of modules on the CLI doesn't matter
  • the important one: every .zig file must exist in at most one module. a file existing in multiple modules will trigger a compile error
  • std, root, and builtin are special modules that exist (importable under those names) to every other module

@mlugg
Copy link
Member

mlugg commented Feb 24, 2023

Does this issue need to remain open? There's no bug here, and I don't actually think there's a usability issue, just people have build scripts written in a way that worked historically but is incorrect.

@leecannon
Copy link
Contributor Author

leecannon commented Feb 25, 2023

The problem i'm hitting now is trying to run tests within modules that depend on themselves, for example the test executable below does not work:

const std = @import("std");

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

    const module = b.createModule(.{
        .source_file = .{ .path = "src/thing.zig" },
    });
    try module.dependencies.put("thing", module);

    const thing_tests = b.addTest(.{
        .root_source_file = .{ .path = "src/thing.zig" },
        .target = target,
        .optimize = optimize,
    });
    thing_tests.addModule("thing", module);

    const test_step = b.step("test", "Run library tests");
    test_step.dependOn(&thing_tests.step);
}
$ zig build test
src/thing.zig:1:1: error: file exists in multiple modules
src/thing.zig:1:1: note: root of module root.thing
src/thing.zig:1:1: note: root of module root

@mlugg
Copy link
Member

mlugg commented Feb 25, 2023

Ah, okay. That's a use case we discussed in the meeting - it's currently not possible, but I'm planning on changes that make it work. Feel free to leave this open to track that problem then (although possibly rename the issue?).

@leecannon
Copy link
Contributor Author

This is possible now after the build system changes in #18160.

@tomc1998
Copy link
Contributor

@leecannon in what way is this now possible after this change?

I am stuck on this exact issue, but cannot understand how the module change fixes it.

    // Create a 'foo' module which is self-referential, e.g. imports 'bar' which imports 'foo' again
    const foo_mod = b.addModule("foo", .{ .root_source_file = b.path("foo.zig") });
    const bar_mod = b.addModule("bar", .{ .root_source_file = b.path("bar.zig") });
    foo_mod.import_table.put(b.allocator, "bar", bar_mod) catch unreachable;
    bar_mod.import_table.put(b.allocator, "foo", foo_mod) catch unreachable;

    // I can't do this, because I get the following error:
    // foo.zig:1:1: error: file exists in multiple modules
    // foo.zig:1:1: note: root of module foo
    // foo.zig:1:1: note: root of module root
    b.addTest(.{ .root_source_file = b.path("foo.zig") });

    // So how can I ever run tests in foo?

The problem is addTest creates a NEW module. there's no way I can use the existing foo module and 'wrap' that to create a test exe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

No branches or pull requests

6 participants