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.

Sign up for GitHub

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

Jump to bottom

Move NIX_BIN_DIR and all logic using it to the Nix executable itself #11178

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Move NIX_BIN_DIR and all logic using it to the Nix executable itself #11178

merged 1 commit into from
Aug 12, 2024

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 25, 2024 •
edited
Loading

Motivation

This is because with the split packages of the Meson build, we simply have no idea what directory the binaries will be installed in when we build the library.

In the process of doing so, consolidate and make more sophisticated the logic to cope with a few corner cases (e.g. NIX_BIN_DIR exists, but no binaries are inside it).

Context

#2503

depends on #11218

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger labels Jul 25, 2024
@Ericson2314 Ericson2314 force-pushed the better-exe-lookup branch 3 times, most recently from df11dd3 to b0bc5cd Compare July 25, 2024 03:59
@Ericson2314 Ericson2314 mentioned this pull request Jul 25, 2024
Merged
fricklerhandwerk
fricklerhandwerk approved these changes Jul 25, 2024
View reviewed changes
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Nice

roberth
roberth reviewed Jul 25, 2024
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Build hook is still a liability. (And reminder to readers, not the post- build hook)

src/libstore/unix/build/hook-instance.cc Outdated Show resolved Hide resolved
src/libstore/globals.cc
If you're porting Nix to a new platform, that might be good enough for a while, but
you'll want to improve `getSelfExe()` to work on your platform.
*/
std::string nixExePath = nixBinDir + "/nix";
Copy link
Member

Choose a reason for hiding this comment

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

This made libstore capable of remote builds regardless of PATH and regardless of the library consumer's main behavior.
If we do this, we need a release note so library consumers can make sure a compatible nix is on their PATH, which is not a given for programs that run as a service.

(fwiw hercules-ci-agent NixOS module seems unaffected, oddly)

@github-actions github-actions bot added documentation store Issues and pull requests concerning the Nix store labels Jul 29, 2024
@Ericson2314 Ericson2314 mentioned this pull request Jul 29, 2024
Merged
@Ericson2314 Ericson2314 force-pushed the better-exe-lookup branch 2 times, most recently from d3676c6 to b7094cc Compare July 29, 2024 21:50
roberth
roberth reviewed Jul 31, 2024
View reviewed changes
Copy link
Member

@roberth roberth left a comment •
edited by Ericson2314
Loading

Choose a reason for hiding this comment

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

Generally looks alright. TODO:

src/libutil/unix/environment-variables.cc Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the better-exe-lookup branch 2 times, most recently from 3e9f429 to 55f301f Compare August 5, 2024 16:13
@Ericson2314 Ericson2314 mentioned this pull request Aug 5, 2024
Open
@Ericson2314 Ericson2314 force-pushed the better-exe-lookup branch 2 times, most recently from d6af41a to 9902e38 Compare August 7, 2024 22:13
roberth
roberth reviewed Aug 7, 2024
View reviewed changes
doc/manual/rl-next/build-hook-default.md Outdated Show resolved Hide resolved
doc/manual/rl-next/build-hook-default.md Outdated Show resolved Hide resolved
doc/manual/rl-next/build-hook-default.md Outdated Show resolved Hide resolved
doc/manual/rl-next/build-hook-default.md Outdated Show resolved Hide resolved
doc/manual/rl-next/build-hook-default.md Outdated Show resolved Hide resolved
src/libcmd/repl.hh Outdated Show resolved Hide resolved
@dpulls dpulls
Copy link

dpulls bot commented Aug 8, 2024

🎉 All dependencies have been resolved !

@roberth
Copy link
Member

roberth commented Aug 8, 2024

This kind of needs a rebase to incorporate the last couple lookup path changes. Note that not all methods were part of #11218; don't drop the first commit (but do fix and reword it)

@Ericson2314
Copy link
Member Author

@roberth

This kind of needs a rebase to incorporate the last couple lookup path changes. Note that not all methods were part of #11218; don't drop the first commit (but do fix and reword it)

I am not sure what this means? It was not rebased on the entirety of #11218 because the second commit of that, but this was a no-conflict rebase on top except for renaming isExecutableAmbient.

@Ericson2314
Copy link
Member Author

Perhaps relating to #11218 (comment) By "second commit" there I had meant "second PR", i.e. this one. The findName vs findPath distinction is done in this one, and findPath is basically what you wrote in your WIP commit.

Also we can decide whether getNixBin should be using any of the ExecutablePath stuff.

roberth
roberth reviewed Aug 10, 2024
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Have thoughts and suggestions, but otherwise LGTM.

src/libutil/executable-path.hh Outdated Show resolved Hide resolved
src/nix-channel/nix-channel.cc
@@ -67,7 +68,7 @@ static void removeChannel(const std::string & name)
channels.erase(name);
writeChannels();

runProgram(settings.nixBinDir + "/nix-env", true, { "--profile", profile, "--uninstall", name });
runProgram(getNixBin("nix-env").string(), true, { "--profile", profile, "--uninstall", name });
Copy link
Member

Choose a reason for hiding this comment

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

We ought to just call C++ for this, but this will do for now.

src/nix/bin-dir.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Is bin-dir the right name for this? It doesn't seem to be about a directory, as it abstracts that away.
Would self-exec be better?

@Ericson2314 @roberth
Move NIX_BIN_DIR and all logic using it to the Nix executable itself
58b03ef 
This is because with the split packages of the Meson build, we simply
have no idea what directory the binaries will be installed in when we
build the library.

In the process of doing so, consolidate and make more sophisticated the
logic to cope with a few corner cases (e.g. `NIX_BIN_DIR` exists, but no
binaries are inside it).

Co-authored-by: Robert Hensing <[email protected]>
@Ericson2314 Ericson2314 merged commit 59def6c into NixOS:master Aug 12, 2024
11 checks passed
@Ericson2314 Ericson2314 deleted the better-exe-lookup branch August 15, 2024 13:26
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-2-25-released/55994/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@L-as L-as L-as left review comments

@roberth roberth roberth left review comments

@fricklerhandwerk fricklerhandwerk fricklerhandwerk approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

Assignees
No one assigned
Labels
documentation new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger store Issues and pull requests concerning the Nix store
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ericson2314 @roberth @nixos-discourse @fricklerhandwerk @L-as