std.Build: fix Compile.installHeader behavior, add WriteFile.addCopyDirectory
#19167
Conversation
| for (wf.directories.items) |dir| { | ||
| man.hash.addBytes(dir.source.getPath2(b, step)); | ||
| man.hash.addBytes(dir.sub_path); | ||
| for (dir.options.exclude_extensions) |ext| man.hash.addBytes(ext); | ||
| if (dir.options.include_extensions) |incs| for (incs) |inc| man.hash.addBytes(inc); | ||
| } |
There was a problem hiding this comment.
This is a bit iffy and won't necessarily catch up with changes to files within the directory, only changes to the installation options. Directory args to Step.Run (which I based this on) have similar issues.
I'm not sure if this is a problem that needs solving or if it's an acceptable tradeoff for not having to recursively hash every single file in a directory tree, but it might be nice to find a solution for this in the future.
|
Additional thoughts: we now have three more or less copy/pasted implementations for "copy directory tree with exclude/include filters" in |
castholm
force-pushed
the
installHeader
branch
2 times, most recently
from
08ea071
to
bc52cd1
Compare
| // 'path' lazy paths are relative to the build root of some step, inferred from the step | ||
| // in which they are used. This means that we can't dupe such paths, because they may | ||
| // come from dependencies with their own build roots and duping the paths as is might | ||
| // cause the build script to search for the file relative to the wrong root. | ||
| // As a temporary workaround, we convert build root-relative paths to absolute paths. | ||
| // If/when the build-root relative paths are updated to encode which build root they are | ||
| // relative to, this workaround should be removed. | ||
| const duped_source: LazyPath = switch (self.source) { | ||
| .path => |root_rel| .{ .cwd_relative = b.pathFromRoot(root_rel) }, | ||
| else => self.source.dupe(b), | ||
| }; |
There was a problem hiding this comment.
I tested this with a simple local project where
- package
aexported a static librarylibbundling build root-relative headera.h, - package
bdepended ona, and - package
bbuilt and installed an executableexewith amain.croot source file that uses symbols froma.hand which did bothexe.linkLibrary(lib)andexe.installLibraryHeaders(lib).
That repro failed with an "unable to update file" error prior to this workaround, but passes now.
If #19313 is accepted and implemented, this workaround can be reverted.
(Thanks @MasterQ32 for indirectly bringing this flaw to my attention.)
Previously, `Step.Compile.installHeader` and friends would incorrectly modify the default `install` top-level step, when the intent was for headers to get bundled with and installed alongside an artifact. This change set implements the intended behavior. This carries with it some breaking changes; `installHeader` and `installConfigHeader` both have new signatures, and `installHeadersDirectory` and `installHeadersDirectoryOptions` have been merged into `installHeaders`.
This is no longer needed after the installed headers refactoring.
Co-authored-by: Abhinav Gupta <[email protected]>
Added test coverage for `installLibraryHeaders`
Also renames `addHeaders` to `addHeadersDirectory` for clarity.
This is a temporary workaround that can be revered if/when 'path' lazy paths are updated to encode which build root they are relative to.
|
Thanks for keeping this updated. Great work. Let's move forward with this and then address any problems in follow-up changes. #19313 is marked as accepted and moved to the 0.12 milestone. |
andrewrk
added
breaking
|
Follow-up issue: #20571 |
Fixes #17204
Closes #19139
The intent for
Compile.installHeaderand friends has always been to bundle the headers alongside an artifact, have them be installed together with the artifact and get automatically added to the include search paths of modules that link with the artifact.Currently, however, these functions modify the default
installtop-level step of the builder, which can lead to a number of unexpected results such as installing or not installing the headers depending on which top-level build steps are invoked.This PR changes it so that installed headers are added to the compile step itself instead of modifying the top-level install step. To handle the construction of the include search path for dependent linking modules, an intermediary
WriteFilestep responsible for constructing the appropriate include tree is created and set up the first time a module links to an artifact.The standalone test
install_headershas been added to verify the correctness of the installation and linking behavior.Summary of changes:
Compile.installHeadernow takes aLazyPath.Compile.installConfigHeaderhas had its second argument removed and now uses the value ofinclude_pathas its sub path, for parity withModule.addConfigHeader. Useartifact.installHeader(config_h.getOutput(), "foo.h")if you want to set the sub path to something different.Compile.installHeadersDirectory/installHeadersDirectoryOptionshave been consolidated intoCompile.installHeadersDirectory, which takes aLazyPathand allows exclude/include filters just likeInstallDir.b.addInstallHeaderFilenow takes aLazyPath.-femit-hheader is now never emitted even when the user specifies an override forh_dir. If you absolutely need the emitted header, you now need to doinstall_artifact.emitted_h = artifact.getEmittedH()until-femit-his fixed.WriteFile.addCopyDirectory, which functions very similar toInstallDir.InstallArtifacthas been updated to install the bundled headers alongide the artifact. The bundled headers are installed to the directory specified byh_dir(which iszig-out/includeby default).