Skip to content

Use the default top-level step for build information #2305

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
wants to merge 1 commit into from

Conversation

arielherself
Copy link

@arielherself arielherself commented May 11, 2025

Currently ZLS cannot correctly resolve @import("root") when there are multiple top-level steps defined in the build script (which is common when say the user needs a test step or something), since it refers to all the root_modules as "root". This leads to errors in the subsequent analysis and wrong derived type affecting inlay hints and auto-completion, because ZLS simply iterates through all packages and find the first one with its name equal to "root".

This PR makes ZLS only analyze the default step, which is often "install", to eliminate such ambiguity. It only does so when build_on_save is not set, so the user could still customize the build commands.

Example to reproduce
  • src/main.zig
    pub const this_should_present = 0;
    pub fn main() void {}
  • src/another_file.zig
    pub const this_should_not_present = 0;
  • build.zig
    const std = @import("std");
    
    pub fn build(b: *std.Build) void {
        const target = b.standardTargetOptions(.{});
        const optimize = b.standardOptimizeOption(.{});
    
        const exe_mod = b.createModule(.{
            .root_source_file = b.path("src/main.zig"),
            .target = target,
            .optimize = optimize,
        });
    
        const exe = b.addExecutable(.{
            .name = "foo",
            .root_module = exe_mod,
        });
    
        b.installArtifact(exe);
    
        const run_cmd = b.addRunArtifact(exe);
    
        run_cmd.step.dependOn(b.getInstallStep());
    
        if (b.args) |args| {
            run_cmd.addArgs(args);
        }
    
        const run_step = b.step("run", "Run the app");
        run_step.dependOn(&run_cmd.step);
    
        const new_mod = b.createModule(.{
            .root_source_file = b.path("src/another_file.zig"),
            .target = target,
            .optimize = optimize,
        });
    
        const exe_unit_tests = b.addTest(.{
            .root_module = new_mod,
        });
    
        const run_exe_unit_tests = b.addRunArtifact(exe_unit_tests);
    
        const test_step = b.step("test", "Run unit tests");
        test_step.dependOn(&run_exe_unit_tests.step);
    }

Result (master branch, commit d697a73):

image

@leecannon
Copy link
Member

leecannon commented May 11, 2025

This results in module imports not working for any module that the default install step does not depend on.

For example https://github.com/CascadeOS/CascadeOS is basically broken without build on save enabled with this change.

@arielherself
Copy link
Author

Oh that's true. And I also realized that even for the default step, we still could not determine what "root" means, because the default step could have multiple compile sub-steps, each having its own root module. It seems impossible to solve this problem properly (adding a search preference, ...) without introducing compilation units.

@arielherself arielherself deleted the inlay-hint-this branch May 11, 2025 15:14
@arielherself
Copy link
Author

arielherself commented May 11, 2025

I think it would be nice to keep the build config per-compile-step, and map each source file to its most relevant build config like CMake does in compile_commands.json. I would try to solve this in a new PR.

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

Successfully merging this pull request may close these issues.

2 participants