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

rename std.Build.FileSource to std.Build.LazyPath #16353

Closed
andrewrk opened this issue Jul 7, 2023 · 5 comments · Fixed by #16446
Closed

rename std.Build.FileSource to std.Build.LazyPath #16353

andrewrk opened this issue Jul 7, 2023 · 5 comments · Fixed by #16446
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Jul 7, 2023

zig/lib/std/Build.zig

Lines 1633 to 1634 in b9fc0d2

/// A file source is a reference to an existing or future file.
pub const FileSource = union(enum) {

It's not really a "file", is it? It could be a directory:

/// Returns a `FileSource` representing the base directory that contains all the
/// files from this `WriteFile`.
pub fn getDirectorySource(wf: *WriteFile) std.Build.FileSource {
return .{ .generated = &wf.generated_directory };
}

"Source" is super confusing. Is it a source file? Not really, it could be an output artifact, or an input binary file.

The doc comment gives it away:

a reference to an existing or future file [or directory]

Therefore I propose:

-/// A file source is a reference to an existing or future file.
-pub const FileSource = union(enum) {
+/// A reference to an existing or future path.
+pub const LazyPath = union(enum) {

As well as all functions such as getOutputSource to getOutput. For that one in particular, let's make it getEmittedBin, corresponding to -femit-bin (related: #16351). Here is a full list of suggested renames:

-std/Build/Step/Run.zig:pub fn addFileSourceArg(self: *Run, file_source: std.Build.FileSource) void {
-std/Build/Step/Run.zig:pub fn addDirectorySourceArg(self: *Run, directory_source: std.Build.FileSource) void {
+std/Build/Step/Run.zig:pub fn addPathArg(self: *Run, path: std.Build.LazyPath) void {

-std/Build/Step/ObjCopy.zig:pub fn getOutputSource(self: *const ObjCopy) std.Build.FileSource {
+std/Build/Step/ObjCopy.zig:pub fn getOutput(self: *const ObjCopy) std.Build.LazyPath {

-std/Build/Step/ConfigHeader.zig:    pub fn getFileSource(style: Style) ?std.Build.FileSource {
+std/Build/Step/ConfigHeader.zig:    pub fn getTemplate(style: Style) ?std.Build.LazyPath {

-std/Build/Step/ConfigHeader.zig:pub fn getFileSource(self: *ConfigHeader) std.Build.FileSource {
+std/Build/Step/ConfigHeader.zig:pub fn getOutput(self: *ConfigHeader) std.Build.LazyPath {

-std/Build/Step/CheckObject.zig:pub fn checkNextFileSource(
+std/Build/Step/CheckObject.zig:pub fn checkNextPath(

-std/Build/Step/Options.zig:pub fn addOptionFileSource(
+std/Build/Step/Options.zig:pub fn addOptionPath(

-std/Build/Step/Options.zig:pub fn getSource(self: *Options) std.Build.FileSource {
+std/Build/Step/Options.zig:pub fn getOutput(self: *Options) std.Build.LazyPath {

-std/Build/Step/WriteFile.zig:    pub fn getFileSource(self: *File) std.Build.FileSource {
+std/Build/Step/WriteFile.zig:    pub fn getPath(self: *File) std.Build.LazyPath {

-std/Build/Step/WriteFile.zig:pub fn getDirectorySource(wf: *WriteFile) std.Build.FileSource {
+std/Build/Step/WriteFile.zig:pub fn getDirectory(wf: *WriteFile) std.Build.LazyPath {

-std/Build/Step/Compile.zig:pub fn addCSourceFile(self: *Compile, file: []const u8, flags: []const []const u8) void {
-std/Build/Step/Compile.zig:pub fn addCSourceFileSource(self: *Compile, source: CSourceFile) void {
+std/Build/Step/Compile.zig:pub fn addCSourceFile(self: *Compile, source: CSourceFile) void {

-std/Build/Step/Compile.zig:pub fn getOutputSource(self: *Compile) std.Build.FileSource {
+std/Build/Step/Compile.zig:pub fn getEmittedBin(self: *Compile) std.Build.LazyPath {

-std/Build/Step/Compile.zig:pub fn getOutputDirectorySource(self: *Compile) std.Build.FileSource {
+std/Build/Step/Compile.zig:pub fn getEmitDirectory(self: *Compile) std.Build.LazyPath {

-std/Build/Step/Compile.zig:pub fn getOutputLibSource(self: *Compile) std.Build.FileSource {
+std/Build/Step/Compile.zig:pub fn getEmittedImplib(self: *Compile) std.Build.LazyPath {

-std/Build/Step/Compile.zig:pub fn getOutputHSource(self: *Compile) std.Build.FileSource {
+std/Build/Step/Compile.zig:pub fn getEmittedH(self: *Compile) std.Build.LazyPath {

-std/Build/Step/Compile.zig:pub fn getOutputPdbSource(self: *Compile) std.Build.FileSource {
+std/Build/Step/Compile.zig:pub fn getEmittedPdb(self: *Compile) std.Build.LazyPath {

-std/Build/Step/Compile.zig:pub fn addAssemblyFileSource(self: *Compile, source: std.Build.FileSource) void {
+std/Build/Step/Compile.zig:pub fn addAssemblyFile(self: *Compile, source: std.Build.LazyPath) void {

-std/Build/Step/Compile.zig:pub fn addObjectFileSource(self: *Compile, source: std.Build.FileSource) void {
+std/Build/Step/Compile.zig:pub fn addObjectFile(self: *Compile, source: std.Build.LazyPath) void {

-std/Build/Step/Compile.zig:pub fn addLibraryPathDirectorySource(self: *Compile, directory_source: std.Build.FileSource) void {
+std/Build/Step/Compile.zig:pub fn addLibraryPath(self: *Compile, path: std.Build.LazyPath) void {

-std/Build/Step/Compile.zig:pub fn addRPathDirectorySource(self: *Compile, directory_source: std.Build.FileSource) void {
+std/Build/Step/Compile.zig:pub fn addRPath(self: *Compile, path: std.Build.LazyPath) void {

-std/Build/Step/Compile.zig:pub fn addFrameworkPathDirectorySource(self: *Compile, directory_source: std.Build.FileSource) void {
+std/Build/Step/Compile.zig:pub fn addFrameworkPath(self: *Compile, path: std.Build.LazyPath) void {

cc @MasterQ32 who originally introduced this crucial abstraction to the build system.

@andrewrk andrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. zig build system std.Build, the build runner, `zig build` subcommand, package management labels Jul 7, 2023
@andrewrk andrewrk added this to the 0.11.0 milestone Jul 7, 2023
@andrewrk andrewrk added contributor friendly This issue is limited in scope and/or knowledge of Zig internals. accepted This proposal is planned. labels Jul 7, 2023
@ikskuh
Copy link
Contributor

ikskuh commented Jul 8, 2023

even if i dont really like LazyPath, i dont have a better name.
looks good to me!

If we perform this change, we maybe should remove the normal "path taking" functions, and also make include and lib dirs use lazypath, but we should add a convenience function for easy physical paths.

Something like

exe.addIncludePath(b.localPath("src/include"));
exe.addLibraryPath(Build.systemPath("/usr/include"));

would be very good for documentation

@andrewrk
Copy link
Member Author

andrewrk commented Jul 8, 2023

even if i dont really like LazyPath, i dont have a better name.

How about FuturePath?

If we perform this change, we maybe should remove the normal "path taking" functions, and also make include and lib dirs use lazypath, but we should add a convenience function for easy physical paths.

Agreed!

Although, it's a bit suspicious to hard code absolute paths to the system, isn't it? It seems to me the build script should merely ask for the system include directory, and then the build system should provide it via a combination of known system directories, and the --search-prefix flags supplied by the user of zig build.

I'm thinking more like this:

zig/lib/std/Build.zig

Lines 1368 to 1412 in 89396ff

pub fn findProgram(self: *Build, names: []const []const u8, paths: []const []const u8) ![]const u8 {
// TODO report error for ambiguous situations
const exe_extension = @as(CrossTarget, .{}).exeFileExt();
for (self.search_prefixes.items) |search_prefix| {
for (names) |name| {
if (fs.path.isAbsolute(name)) {
return name;
}
const full_path = self.pathJoin(&.{
search_prefix,
"bin",
self.fmt("{s}{s}", .{ name, exe_extension }),
});
return fs.realpathAlloc(self.allocator, full_path) catch continue;
}
}
if (self.env_map.get("PATH")) |PATH| {
for (names) |name| {
if (fs.path.isAbsolute(name)) {
return name;
}
var it = mem.tokenizeScalar(u8, PATH, fs.path.delimiter);
while (it.next()) |path| {
const full_path = self.pathJoin(&.{
path,
self.fmt("{s}{s}", .{ name, exe_extension }),
});
return fs.realpathAlloc(self.allocator, full_path) catch continue;
}
}
}
for (names) |name| {
if (fs.path.isAbsolute(name)) {
return name;
}
for (paths) |path| {
const full_path = self.pathJoin(&.{
path,
self.fmt("{s}{s}", .{ name, exe_extension }),
});
return fs.realpathAlloc(self.allocator, full_path) catch continue;
}
}
return error.FileNotFound;
}

@ikskuh
Copy link
Contributor

ikskuh commented Jul 8, 2023

How about FuturePath?

Yeah i think this is more fitting. I mean, it's basically a Promise<Path> anyways, so we could also go with PathPromise.
After reading Futures and promises, it's definitly a FuturePath :)

Although, it's a bit suspicious to hard code absolute paths to the system, isn't it?

Yeah, but it's good for getting stuff up and running when dealing with several packages/tools that aren't on a remote yet and i don't copy-paste/link it into all my projects.

Imho system-wide paths should be supported for a good local-development experience, but that's up to you. the search prefix thing looks good.

@nektro
Copy link
Contributor

nektro commented Jul 8, 2023

LazyPath nomenclature has precedent in LazySrcLoc fwiw

@ikskuh
Copy link
Contributor

ikskuh commented Jul 19, 2023

trying to tackle that, going with LazyPath

andrewrk added a commit that referenced this issue Jul 24, 2023
 * Allow calling it multiple times.
 * Rename it. Sorry, this is to coincide with #16353.
andrewrk added a commit that referenced this issue Jul 24, 2023
 * Allow calling it multiple times.
 * Rename it. Sorry, this is to coincide with #16353.
ikskuh pushed a commit to ikskuh/zig that referenced this issue Jul 25, 2023
…yPath and removes functions that take literal paths instead of LazyPath.
ikskuh pushed a commit to ikskuh/zig that referenced this issue Jul 26, 2023
…yPath and removes functions that take literal paths instead of LazyPath.
andrewrk pushed a commit to ikskuh/zig that referenced this issue Jul 30, 2023
…yPath and removes functions that take literal paths instead of LazyPath.
andrewrk pushed a commit to ikskuh/zig that referenced this issue Jul 30, 2023
…yPath and removes functions that take literal paths instead of LazyPath.
QusaiHroub pushed a commit to QusaiHroub/zig that referenced this issue Aug 3, 2023
…yPath and removes functions that take literal paths instead of LazyPath.
chrboesch pushed a commit to ziglings-org/exercises that referenced this issue Jan 5, 2024
tacogips pushed a commit to tacogips/zigling-excersize that referenced this issue Feb 12, 2024
jesseb34r pushed a commit to jesseb34r/ziglings that referenced this issue Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants