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-symlinks-relative: fix no such file or directory if output is cr… #204692

Merged
merged 2 commits into from Dec 9, 2022

Conversation

Artturin
Copy link
Member

@Artturin Artturin commented Dec 5, 2022

…eated in postFixup

wlroots(and others) have

wlroots> +++ find /nix/store/3a0xwszw8n5dzzhsgfnilvsqi4hk565s-wlroots-0.15.1-examples -type l -print0
wlroots> find: '/nix/store/3a0xwszw8n5dzzhsgfnilvsqi4hk565s-wlroots-0.15.1-examples': No such file or directory

because the examples output is created in postFixup while this hook runs in fixupPhase

Description of changes
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/)
  • 23.05 Release Notes (or backporting 22.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.

…eated in postFixup

wlroots(and others) have
```
wlroots> +++ find /nix/store/3a0xwszw8n5dzzhsgfnilvsqi4hk565s-wlroots-0.15.1-examples -type l -print0
wlroots> find: '/nix/store/3a0xwszw8n5dzzhsgfnilvsqi4hk565s-wlroots-0.15.1-examples': No such file or directory
```

because the examples output is created in postFixup while this hook runs in fixupPhase
@andir
Copy link
Member

andir commented Dec 6, 2022

Maybe those builds are wrong then? Isn't it rather postBuild or postInstall or installPhase where outputs should be written? Doing that in fixup sounds wrong.

@Artturin
Copy link
Member Author

Artturin commented Dec 6, 2022

Maybe those builds are wrong then? Isn't it rather postBuild or postInstall or installPhase where outputs should be written? Doing that in fixup sounds wrong.

Yeah maybe but wouldn't it be better for this hook to run on symlinks added in postFixup

Afaik there have been no issues caused by this hook

@Artturin
Copy link
Member Author

Artturin commented Dec 7, 2022

@andir anymore questions or do you think this is ready to merge?

@Artturin
Copy link
Member Author

Artturin commented Dec 8, 2022

@ofborg build sway zsh

@andir
Copy link
Member

andir commented Dec 9, 2022

@andir anymore questions or do you think this is ready to merge?

I haven't tested this, so I can't really give a +1 just yet. I'll try to test this on the weekend.

@Artturin
Copy link
Member Author

Artturin commented Dec 9, 2022

i added a test for the hook

$ nix build -f . "tests.hooks"
this derivation will be built:
  /nix/store/askavvq9j724816kv9j5dkncp7c4z0p3-test-make-symlinks-relative.drv
building '/nix/store/askavvq9j724816kv9j5dkncp7c4z0p3-test-make-symlinks-relative.drv'...
test-make-symlinks-relative> symlink before patching: /nix/store/agabiwibxpvkfl4aa3ilp7v1mimxi744-test-make-symlinks-relative/bar/foo
test-make-symlinks-relative> rewriting symlink /nix/store/agabiwibxpvkfl4aa3ilp7v1mimxi744-test-make-symlinks-relative/baz/foo to be relative to /nix/store/agabiwibxpvkfl4aa3ilp7v1mimxi744-test-make-symlinks-relative
test-make-symlinks-relative> symlink after patching: ../bar/foo
test-make-symlinks-relative> symlink isn't broken
test-make-symlinks-relative> absolute symlink was made relative

@Artturin
Copy link
Member Author

Artturin commented Dec 9, 2022

also tested with

nix build ".#bzip2^*"
...
bzip2> rewriting symlink /nix/store/8dvdlr36j65b684ba1r160pdya6i329f-bzip2-1.0.8/lib/libbz2.so.1.0 to be relative to /nix/store/8dvdlr36j65b684ba1r160pdya6i329f-bzip2-1.0.8

$ \ls -l ./result/lib
total 43
-r-xr-xr-x  2 root root   946  1. 1.  1970 libbz2.la
lrwxrwxrwx 67 root root    15  1. 1.  1970 libbz2.so -> libbz2.so.1.0.8
lrwxrwxrwx 67 root root    15  1. 1.  1970 libbz2.so.1 -> libbz2.so.1.0.8
lrwxrwxrwx 67 root root    15  1. 1.  1970 libbz2.so.1.0 -> libbz2.so.1.0.8
-r-xr-xr-x  2 root root 79072  1. 1.  1970 libbz2.so.1.0.8

with dontRewriteSymlinks = true;

$ \ls -l ./result/lib
total 51
-r-xr-xr-x  2 root root   946  1. 1.  1970 libbz2.la
lrwxrwxrwx 69 root root    15  1. 1.  1970 libbz2.so -> libbz2.so.1.0.8
lrwxrwxrwx 69 root root    15  1. 1.  1970 libbz2.so.1 -> libbz2.so.1.0.8
lrwxrwxrwx  2 root root    75  1. 1.  1970 libbz2.so.1.0 -> /nix/store/fd3jygv5iflmdflkwr9g0gwg79v7r65q-bzip2-1.0.8/lib/libbz2.so.1.0.8
-r-xr-xr-x  2 root root 78232  1. 1.  1970 libbz2.so.1.0.8

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Dec 9, 2022

Maybe those builds are wrong then? Isn't it rather postBuild or postInstall or installPhase where outputs should be written? Doing that in fixup sounds wrong.

Then we should at least error out in such scenarios instead of silently continuing.

@andir
Copy link
Member

andir commented Dec 9, 2022

Maybe those builds are wrong then? Isn't it rather postBuild or postInstall or installPhase where outputs should be written? Doing that in fixup sounds wrong.

Then we should at least error out in such scenarios instead of silently continuing.

Feel free. I'll yield on this issue. You seem to know what you are doing.

Don't mention me in this PR again. Thanks.

@Artturin
Copy link
Member Author

Artturin commented Dec 9, 2022

Feel free. I'll yield on this issue. You seem to know what you are doing.

Don't mention me in this PR again. Thanks.

Alright, thanks for the hook.

@Artturin
Copy link
Member Author

Artturin commented Dec 9, 2022

@ofborg build tests.hooks

@Artturin
Copy link
Member Author

Artturin commented Dec 9, 2022

Mention me if there are issues

@Artturin Artturin merged commit 91d19a6 into NixOS:staging Dec 9, 2022
@Artturin Artturin deleted the relative-links-fix-error branch December 9, 2022 17:27
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