Skip to content

Make getExe (writeScript x y) work#173675

Draft
roberth wants to merge 1 commit intoNixOS:masterfrom
hercules-ci:getExe-writeScript-2
Draft

Make getExe (writeScript x y) work#173675
roberth wants to merge 1 commit intoNixOS:masterfrom
hercules-ci:getExe-writeScript-2

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 from 486be9e to f8a3625 Compare May 19, 2022 22:07
@roberth roberth force-pushed the getExe-writeScript-2 branch 2 times, most recently from 7c8e404 to 4c5fa86 Compare June 14, 2022 21:45
@roberth roberth force-pushed the getExe-writeScript-2 branch from 4c5fa86 to c9bf253 Compare June 21, 2022 23:34
@roberth roberth force-pushed the getExe-writeScript-2 branch from c9bf253 to df0e96b Compare June 21, 2022 23:35
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 21, 2022
@roberth roberth marked this pull request as ready for review June 22, 2022 08:07
@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 This PR has merge conflicts with the target branch 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
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants