std.Build: some miscelleanous bits
#24205
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This function is sometimes used to assume a canonical representation of a path. However, when the `Path` referred to `root_dir` itself, this function previously resolved `sub_path` to ".", which is incorrect; per doc comments, it should set `sub_path` to "". This fix ultimately didn't solve what I was trying to solve, though I'm still PRing it, because it's still *correct*. The background to this commit is quite interesting and worth briefly discussing. I originally worked on this to try and fix a bug in the build system, where if the root package (i.e. the one you `zig build`) depends on package X which itself depends back on the root package (through a `.path` dependency), invalid dependency modules are generated. I hit this case working on ziglang/translate-c, which wants to depend on "examples" (similar to the Zig compiler's "standalone" test cases) which themselves depend back on the translate-c package. However, after this patch just turned that error into another, I realised that this case simply cannot work, because `std.Build` needs to eagerly execute build scripts at `dependency` calls to learn which artifacts, modules, etc, exist. ...at least, that's how the build system is currently designed. One can imagine a world where `dependency` sort of "queues" the call, `artifact` and `module` etc just pretend that the thing exists, and all configure functions are called non-recursively by the runner. The downside is that it becomes impossible to query state set by a dependency's configure script. For instance, if a dependency exposes an artifact, it would become impossible to get that artifact's resolved target in the configure phase. However, as well as allowing recursive package imports (which are certainly kinda nifty), it would also make lazy dependencies far more useful! Right now, lazy dependencies only really work if you use options (`std.Build.option`) to block their usage, since any call to `lazyDependency` causes the dependency to be fetched. However, if we made this change, lazy dependencies could be made far more versatile by only fetching them *if the final step plan requires them*. I'm not 100% sure if this is a good idea or not, but I might open an issue for it soon.
`std.Build.Step.ConfigHeader` emits a *directory* containing a config header under a given sub path, but there's no good way to actually access that directory as a `LazyPath` in the configure phase. This is silly; it's perfectly valid to refer to that directory, perhaps to explicitly pass as a "-I" flag to a different toolchain invoked via a `Step.Run`. So now, instead of the `GeneratedFile` being the actual *file*, it should be that *directory*, i.e. `cache/o/<digest>`. We can then easily get the *file* if needed just by using `LazyPath.path` to go "deeper", which there is a helper function for. The legacy `getOutput` function is now a deprecated alias for `getOutputFile`, and `getOutputDir` is introduced. `std.Build.Module.IncludeDir.appendZigProcessFlags` needed a fix after this change, so I took the opportunity to refactor it a little. I was looking at this function while working on ziglang/translate-c yesterday and realised it could be expressed much more simply -- particularly after the `ConfigHeader` change here. I had to update the test `standalone/cmakedefine/` -- it turns out this test was well and truly reaching into build system internals, and doing horrible not-really-allowed stuff like overriding the `makeFn` of a `TopLevelStep`. To top it all off, the test forgot to set `b.default_step` to its "test" step, so the test never even ran. I've refactored it to follow accepted practices and to actually, like, work.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One bugfix (which I don't think actually really affected anything), and one new feature. See commit messages.