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

Step.Compile.installHeader and related functions don't install headers as intended #17204

Closed
castholm opened this issue Sep 19, 2023 · 3 comments · Fixed by #19167
Closed

Step.Compile.installHeader and related functions don't install headers as intended #17204

castholm opened this issue Sep 19, 2023 · 3 comments · Fixed by #19167
Labels
bug Observed behavior contradicts documented or intended behavior zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@castholm
Copy link
Contributor

Zig Version

0.12.0-dev.409+48e2ba3b3

Steps to Reproduce and Observed Behavior

Step.Compile.installHeader, Step.Compile.installLibraryHeaders and similar functions are intended to mark installing the headers a dependency of installing the compiled artifact, i.e. the headers are bundled with the compile step and are automatically installed when the compiled artifact is installed.

Currently, however, these functions add themselves to the default install top-level step (b.install_tls) instead of registering themselves with the compile step, which results in the headers not being installed if the compiled artifact is installed via a custom top-level step.

If I have the following code in my build.zig

const foo = b.addStaticLibrary(.{
    .name = "foo",
    .target = target,
    .optimize = optimize,
});
foo.installHeader("hello.h", "hello.h");

const my_step = b.step("my_step", "...");
my_step.dependOn(&b.addInstallArtifact(foo, .{}).step);

and run zig build my_step, it results in the following files being installed

zig-out/
+-- lib/
    +-- foo.lib

with hello.h missing.

The affected Step.Compile functions are:

  • installHeader
  • installConfigHeader
  • installHeadersDirectory
  • installHeadersDirectoryOptions
  • installLibraryHeaders

Expected Behavior

The affected functions should bundle the headers with the installed artifacts instead of modifying the global top-level install step.

Given the example input, zig build my_step should install the following files:

zig-out/
+-- include/
|   +-- hello.h
+-- lib/
    +-- foo.lib
@castholm castholm added the bug Observed behavior contradicts documented or intended behavior label Sep 19, 2023
@castholm castholm changed the title Step.Compile.installHeader and related functions don't work as intended Step.Compile.installHeader and related functions don't install headers as intended Sep 19, 2023
@andrewrk andrewrk added the zig build system std.Build, the build runner, `zig build` subcommand, package management label Sep 19, 2023
@andrewrk andrewrk added this to the 0.12.0 milestone Sep 19, 2023
abhinav added a commit to abhinav/zig that referenced this issue Mar 1, 2024
Currently, `installHeader`, `installConfigHeader`, and friends all
add the installation step as a dependency of the root install step.
This means that if a library is installed with a custom top-level step,
then the headers are not installed.

To fix this, this commit changes these functions to instead have the
compile step depend on the header installation stpe.
This way, as long as the compile step for an artifact is consumed,
the headers will be installed.

Two sets of tests are included to verify this behavior:

- A unit test in std.Build.Step.Compile that verifies that dependencies
  are correctly connected.
- A standalone test that reproduces the issue reported in ziglang#17204, and
  verifies that the headers are installed even without the install step.

Resolves ziglang#17204
@abhinav
Copy link
Contributor

abhinav commented Mar 1, 2024

I put up #19139 based on the issue description, but I'm not certain about this behavior.
With that PR, an artifact will install its headers to zig-out/include when it's built even if the library is never installed. That doesn't seem right.

I think the most correct form of this should install the headers only if someone calls addInstallArtifact(lib) (as a dependency of that install step) but I don't see an easy way to do that. There's no other concept of a non-global install step available to the Compile step.
We somehow need the InstallArtifact step from addInstallArtifact to be plumbed to the install operations in Compile.

@castholm
Copy link
Contributor Author

castholm commented Mar 2, 2024

@abhinav I made a brief attempt at trying to solve this a few weeks ago by changing it so that Step.Compile.installed_headers stores headers in a way that doesn't create additional install steps or modifies the default top-level step. I got a bit distracted and never ended up finishing it. Would you mind if I completed that work, adapted your tests and opened a PR for it?

@abhinav
Copy link
Contributor

abhinav commented Mar 2, 2024

@castholm no, absolutely not. Go for it!

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 zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
3 participants