Skip to content

std: allow overriding install path of artifacts #6314

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

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

ifreund
Copy link
Member

@ifreund ifreund commented Sep 10, 2020

This is necessary when, for example, writing a PAM module which should
be installed to lib/security/module_name.so.

Currently the zig build system forces shared libraries to be prefixed
with "lib" and versions the so files as well as creating symlinks.
Furthermore, artifacts can currently only be installed to the bin or
lib directories, not sub-directories thereof.

Allowing the user to manually specify the install path allows maximum
flexibility, solving this use case and more.

@ifreund
Copy link
Member Author

ifreund commented Sep 10, 2020

This feature would allow me to merge this commit replacing my current Makefile with a build.zig. However, the current implementation may be considered a little heavy-handed, suggestions on how to improve it are of course welcome.

@ifreund ifreund force-pushed the install-path-override branch from d5f1f8a to a2fa15e Compare September 10, 2020 21:51
@andrewrk
Copy link
Member

andrewrk commented Sep 10, 2020

I definitely want to solve this use case and unblock you - thank you for bringing this up. I haven't looked very deeply into this, but what are your thoughts on a different approach of extending InstallDir to support the security subdir?

zig/lib/std/build.zig

Lines 2618 to 2623 in 800c5de

pub const InstallDir = enum {
Prefix,
Lib,
Bin,
Header,
};

@ifreund
Copy link
Member Author

ifreund commented Sep 10, 2020

I definitely want to solve this use case and unblock you - thank you for bringing this up. I haven't looked very deeply into this, but what are your thoughts on a different approach of extending InstallDir to support the security subdir?

Hmm, I don't think that we should have to teach the zig build system about every possible sub directory that one might want to install to, instead there should be a way to take control and install artifacts to any arbitrary path.

That also wouldn't solve the issue with the naming of the so file, but I see now that #2231 and #2230 would.

As a way to move forward with this I think I'll give implementing those a go and then revisit this.

@andrewrk
Copy link
Member

Hmm, I don't think that we should have to teach the zig build system about every possible sub directory that one might want to install to, instead there should be a way to take control and install artifacts to any arbitrary path.

That makes sense - what about making InstallDir a tagged union and adding a "custom" tag?

This is necessary when, for example, writing a PAM module which should
be installed to lib/security/module_name.so.
@ifreund ifreund force-pushed the install-path-override branch from a2fa15e to f78e4b1 Compare September 11, 2020 00:00
@@ -1212,6 +1213,8 @@ pub const LibExeObjStep = struct {
is_linking_libc: bool = false,
vcpkg_bin_path: ?[]const u8 = null,

/// This may be set in order to override the default install directory
override_dest_dir: ?InstallDir,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't really seem like a function to set this variable is necessary, users of the api can just set the field directly.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - and you made sure this solves your use case in particular, right?

.Obj => unreachable,
.Test => unreachable,
.Exe => .Bin,
.Lib => .Lib,
.Exe => InstallDir{ .Bin = {} },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're working around a stage1 issue here, but just to let you know it's planned for .Bin to coerce to the tagged union here, which would make these kind of code reworkings nicer in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that seems to be the case unfortunately, I played with it a bit but this was the cleanest it got.

@ifreund
Copy link
Member Author

ifreund commented Sep 11, 2020

Looks good - and you made sure this solves your use case in particular, right?

It will once #2230 and #2231 are solved, with just this PR my build.zig installs to lib/security/libpam_rundird.so.0.0.0 (plus symlinks) while what I need in the end is lib/security/pam_rundird.so.

#6315 makes further progress towards this.

@andrewrk andrewrk merged commit 68bf29c into ziglang:master Sep 11, 2020
@ifreund ifreund deleted the install-path-override branch September 11, 2020 08:47
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