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

std.Build.Module.addShallowDependencies: fix missing dependencies creation #21936

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Lzard
Copy link
Contributor

@Lzard Lzard commented Nov 8, 2024

Fixes dependencies of the following items not being added to importing modules:

  • all include_dirs
  • root of c_source_files link_objects
  • include_paths of win32_resource_file link_objects

Example of missing ConfigHeader dependencies

Given this build.zig:

const std = @import("std");

pub fn build(b: *std.Build) void {
    const cfgh = b.addConfigHeader(.{ .style = .blank }, .{});

    const mod = b.createModule(.{
        .root_source_file = b.path("src/root.zig"),
    });
    mod.addConfigHeader(cfgh);

    const exe = b.addExecutable(.{
        .name = "exe",
        .root_source_file = b.path("src/main.zig"),
        .target = b.standardTargetOptions(.{}),
        .optimize = b.standardOptimizeOption(.{}),
    });
    exe.root_module.addImport("", mod);

    b.getInstallStep().dependOn(&exe.step);
}
  • zig build without this PR:
thread 7488 panic: getPath() was called on a GeneratedFile that wasn't built yet. Is there a missing Step dependency on step 'configure blank header to config.h'?
  • zig build with this PR compiles successfully.

@BratishkaErik
Copy link
Contributor

BratishkaErik commented Nov 9, 2024

While you are in this code, can you please also port everything from this commit d873dd4 from PR #20388? After that addShallowDependencies should cover everything in module.

std.Build.Module.addShallowDependencies: fix missing dependencies

De-syncing here was not exposed before because old artifact creation API
always called addShallowDependencies before user could call
addCSourceFile etc., and their logic constantly overwrote this one.

Example of this de-syncing: addShallowDependencies depended only
on RcSourceFile.file LazyPath, but did not depend on
RcSourceFile.include_paths LazyPaths. Meanwhile,
addWin32ResourceFile depended on both.

This means that user could not call addWin32ResourceFile this way:

const mod = b.createModule(...);
mod.addWin32ResourceFile(...);
const exe = b.addExecutable2(...);

Since addShallowDependencies will be called on 1 and 3 line, with
incomplete logic, and Step.Compile will miss include_paths dependency.

I initially tried to do this in standalone test "windows_resources",
but it caused errors about file not being generated yet:

getPath() was called on a GeneratedFile that wasn't built yet.
  source package path: /home/bratishkaerik/github.com/zig/test/standalone/windows_resources
  Is there a missing Step dependency on step 'WriteFile generated.h'?
    The step was created by this stack trace:
name: 'WriteFile generated.h'. creation stack trace:

Now addShallowDependencies is synced with all functions and dependecies
are same no matter if user calls them before b.addExecutable2 or after it.

UPD: except that part with "old API/new API". I'm asking because your PR have better chances to be merged sooner and fix all errors at once.

@Lzard Lzard force-pushed the build-module-include-dependencies branch 3 times, most recently from 7a03b24 to 414eb42 Compare November 9, 2024 16:47
@Lzard Lzard force-pushed the build-module-include-dependencies branch from 414eb42 to 27b8f64 Compare November 9, 2024 16:58
@Lzard Lzard changed the title std.Build.Module: add include_dirs shallow dependencies std.Build.Module.addShallowDependencies: fix missing dependencies creation Nov 9, 2024
@Lzard
Copy link
Contributor Author

Lzard commented Nov 9, 2024

While you are in this code, can you please also port everything from this commit d873dd4 from PR #20388?

The changes from that commit that were missing have been added.

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