Skip to content

build: module imports do not respect target OS #20492

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
cbilz opened this issue Jul 3, 2024 · 3 comments
Closed

build: module imports do not respect target OS #20492

cbilz opened this issue Jul 3, 2024 · 3 comments

Comments

@cbilz
Copy link
Contributor

cbilz commented Jul 3, 2024

Zig Version

0.14.0-dev.144+a31fe8aa3

Steps to Reproduce and Observed Behavior

In the following example, a Linux executable is allowed to import a module targeting WASI.

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

pub fn build(b: *std.Build) void {
    const foreign = b.addModule("foreign", .{
        .root_source_file = b.path("src/foreign.zig"),
        .target = b.resolveTargetQuery(.{ .os_tag = .wasi }),
    });
    const exe = b.addExecutable(.{
        .name = "main",
        .root_source_file = b.path("src/main.zig"),
        .target = b.standardTargetOptions(.{}),
    });
    exe.root_module.addImport("foreign", foreign);

    const run_step = b.step("run", "Run the executable");
    run_step.dependOn(&b.addRunArtifact(exe).step);
}
// src/foreign.zig
pub const os = @import("builtin").os;
// src/main.zig
const std = @import("std");

pub fn main() void {
    std.debug.print("builtin os: {}\n", .{ @import("builtin").os.tag });
    std.debug.print("foreign os: {}\n", .{ @import("foreign").os.tag });
}
$ zig-linux-x86_64-0.14.0-dev.144+a31fe8aa3/zig build run
builtin os: Target.Os.Tag.linux
foreign os: Target.Os.Tag.wasi

Related Ziggit topic

Expected Behavior

I see two problems with this behavior:

  • The imported module cannot observe the actual compilation options because builtin does not behave as one might expect.
  • The intended compilation options of the imported module are ignored. So the Module.CreateOptions argument of addModule does not do what one might expect.

I think the build system should do one of the following:

  • ensure that any overrides of compilation options are reflected in builtin, or
  • refuse to import modules with different compilation options. For instance, refuse to import if anything in builtin will differ between the importing and the imported modules.

I don't know which is better, partly because I don't understand why Module was designed to carry compilation options in the first place.

@cbilz cbilz added the bug Observed behavior contradicts documented or intended behavior label Jul 3, 2024
@cbilz cbilz changed the title build: module imports do not respect targets build: module imports do not respect compilation options Jul 3, 2024
@BratishkaErik
Copy link
Contributor

IIRC this is intended behaviour that different modules can have different builtin modules for them, see #18160 . If you want same target, you need to pass same target :)

@cbilz
Copy link
Contributor Author

cbilz commented Jul 4, 2024

intended behaviour that different modules can have different builtin modules for them, see #18160

I see, thank you for linking the correct pull request! It seems the motivation is that, quoting from #18160:

modules can be compiled with different optimization modes, code model, valgrind integration, or even target CPU feature set.

It does indeed seem useful to be able to vary these compilation options across different modules. However, I think this doesn't apply to all compilation options. Specifically, I would consider the target OS a per-compilation option rather than a per-module option. Let me rephrase the two problems mentioned in the original comment:

  • The imported module cannot detect the actual target OS via builtin, because compilation overrides the target OS.
  • The intended target OS of the imported module is ignored. In other words, setting the OS through addModule does not protect against OS-incompatible imports.

Some other options also look like natural per-compilation options, e.g. CPU architecture, object format and PIC.

EDIT:

Closely related question in #14719:

I'm not sure which APIs should be copied from CompileStep. All of them? Just ones related to installing headers?

From #14731 (review), emphasis mine:

Move all logic from CompileStep into Module that makes sense to go there

Perhaps too much was moved?

@cbilz cbilz changed the title build: module imports do not respect compilation options build: module imports do not respect target OS Jul 4, 2024
@andrewrk
Copy link
Member

Working As Designed

@andrewrk andrewrk closed this as not planned Won't fix, can't repro, duplicate, stale Aug 12, 2024
@andrewrk andrewrk removed the bug Observed behavior contradicts documented or intended behavior label Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants