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

Make getExe (writeScript x y) work #173675

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roberth
Copy link
Member

@roberth roberth commented May 19, 2022

Description of changes

Broken, because path is missing:

nix-repl> lib.getExe (writeScript "hi" "hello")
"/nix/store/q8ybdl7z2v0sz0r6y9pkxb1bjk5xf0zd-hi/bin/hi"

After PR:

nix-repl> lib.getExe (writeScript "hi" "hello")
"/nix/store/q8ybdl7z2v0sz0r6y9pkxb1bjk5xf0zd-hi"
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@roberth roberth mentioned this pull request May 19, 2022
13 tasks
@roberth roberth force-pushed the getExe-writeScript-2 branch 2 times, most recently from 7c8e404 to 4c5fa86 Compare June 14, 2022 21:45
@Artturin
Copy link
Member

Artturin commented Dec 21, 2022

this wouldn't match the behavior of nix run.

what if we instead check if meta.mainProgram is an absolute path and use that if so

that'll need a change in
https://github.com/NixOS/nix/blob/b1223e1b621ed4ad11562f0ef65c65d1c78c5e4b/src/nix/app.cc#L93-L112

opinions?

@Artturin Artturin marked this pull request as draft December 21, 2022 12:50
@roberth
Copy link
Member Author

roberth commented Dec 21, 2022

So it looks like nix run would need to change in any case.

what if we instead check if meta.mainProgram is an absolute path and use that if so

I think there's an unwritten rule that meta should not contain store references.
Also the default executable location is normal data that we use in normal expressions, not metadata.

opinions?

Another way to approach this, is to make writeScript return a writeScriptBin package with outPath set to the main executable. Single-file store paths are a forward compatibility mistake anyway, so maybe we should be thinking in this direction? Then nix run could check whether outPath is a store path with subpath, and if so, run that.

@roberth
Copy link
Member Author

roberth commented Jul 31, 2023

It might be preferable for writeScript to return a writeScriptBin x y + "/bin/${x}".
Unfortunately overrideAttrs doesn't seem capable of resetting outPath based on outPath, so that overrideAttrs would keep working as is. We could either add finalDerivationOutputs next to finalPackage, but my preferred option would be to factor out a mkPackage function that would allows us to construct the right behavior without much of the legacy and without the complexity of the finalPackage solution. I'll save that for when we have RFC 92 Dynamic Derivations though.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants