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

gnulib: Fix path to perl in prefix-gnulib-mk #274161

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

afh
Copy link
Member

@afh afh commented Dec 14, 2023

Description of changes

While building a custom groff using nixpkgs I came across the following error:

/nix/store/cic1hyg92ggj4swjp37phggvdacq2kz6-source/build-aux/prefix-gnulib-mk: line 26: exec: perl: not found

Inspecing prefix-gnulib-mk perl is used in line 26. Hence replacing perl with a proper path to perl in nix seems necessary.

❓ Please let me know if you believe this PR should rather be applied to master than staging. Since it

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Dec 18, 2023

Did we before rely on perl being in PATH or did we just never hit that code path?

Also this should go to master, since it rebuilds only ~10 or so packages.

@afh afh changed the base branch from staging to master December 19, 2023 12:05
@afh
Copy link
Member Author

afh commented Dec 19, 2023

Thank you for your feedback, @SuperSandro2000, this PR's merge base has now been changed to master.

Unfortunately I cannot answer that question with confidence, I assume that we never hit that code path. I only came across it while trying to manually build groff from within a nix develop shell.

@SuperSandro2000
Copy link
Member

I think if we can keep relying on PATH we can keep the build closures of all other packages smaller which would be need but perl is probably already somewhere in there

@afh
Copy link
Member Author

afh commented Dec 19, 2023

Happy to look for alternative solutions, @SuperSandro2000 and maybe you have time and interest to partake in finding them.

Inspecting gnulib's build-aux/prefix-gnulib-mk:26 it runs eval 'exec perl -wSx "$0" "$@"' for some reason unclear to me the following error appears while trying to build groff from within a nix develop shell: /nix/store/cic1hyg92ggj4swjp37phggvdacq2kz6-source/build-aux/prefix-gnulib-mk: line 26: exec: perl: not found.

Inside and outside that build environment perl is /usr/bin/perl. Is there anything from nixpkgs that comes to your mind, that would change PATH in a way that it is no longer found by prefix-gnulib-mk?

Is there a way in nixpkgs with which perl could be substituted for a valid absolute path to the perl executable on the current system (for me on macos /usr/bin/perl)?

What are some of the things you'd try?

@SuperSandro2000
Copy link
Member

Is there anything from nixpkgs that comes to your mind, that would change PATH in a way that it is no longer found by prefix-gnulib-mk?

In the sandbox it is cleared.

Is there a way in nixpkgs with which perl could be substituted for a valid absolute path to the perl executable on the current system (for me on macos /usr/bin/perl)?

We should probably replace the shebang on line 1 with a perl shebang to avoid this hackery.

@afh
Copy link
Member Author

afh commented Jan 3, 2024

Thanks for the feedback and suggestion, @SuperSandro2000.
Is there another way you can think of that would help with keeping the build closures smaller as you requested earlier?

@afh
Copy link
Member Author

afh commented Jan 11, 2024

I'd like to post this PR in the "PRs already reviewed" channel and kindly request re-review from you, @SuperSandro2000, as you've taken at look at this previously.

@pbsds
Copy link
Contributor

pbsds commented Jan 11, 2024

Result of nixpkgs-review pr 274161 run on x86_64-linux 1

32 packages built:
  • gnu-pw-mgr
  • gnulib
  • idutils
  • lbzip2
  • lua51Packages.lrexlib-gnu
  • luaPackages.lrexlib-gnu (lua52Packages.lrexlib-gnu)
  • lua53Packages.lrexlib-gnu
  • lua54Packages.lrexlib-gnu
  • luajitPackages.lrexlib-gnu
  • python310Packages.condaInstallHook
  • python310Packages.torchWithRocm
  • python310Packages.torchWithRocm.cxxdev
  • python310Packages.torchWithRocm.dev
  • python310Packages.torchWithRocm.dist
  • python310Packages.torchWithRocm.lib
  • python311Packages.condaInstallHook
  • python311Packages.torchWithRocm
  • python311Packages.torchWithRocm.cxxdev
  • python311Packages.torchWithRocm.dev
  • python311Packages.torchWithRocm.dist
  • python311Packages.torchWithRocm.lib
  • rocmPackages.migraphx (rocmPackages_5.migraphx)
  • rocmPackages.miopen (rocmPackages.miopen-hip ,rocmPackages_5.miopen ,rocmPackages_5.miopen-hip)
  • rocmPackages.miopen-opencl (rocmPackages_5.miopen-opencl)
  • rocmPackages.mivisionx (rocmPackages.mivisionx-hip ,rocmPackages_5.mivisionx ,rocmPackages_5.mivisionx-hip)
  • rocmPackages.mivisionx-cpu (rocmPackages_5.mivisionx-cpu)
  • rocmPackages.mivisionx-opencl (rocmPackages_5.mivisionx-opencl)
  • surf-display
  • wget2
  • wget2.dev
  • wget2.lib
  • xprintidle-ng

Shebang is properly patched as intended, and the executable seems to work. LGTM

This seems to produce the only in-store reference to perl, which means the package previously was non-hermetic. There are other occurrences of files which tries to exec into perl from sh, if this is the direction you want to go, please patch them as well

build-aux/useless-if-before-free
build-aux/update-copyright
build-aux/gitlog-to-changelog
build-aux/announce-gen

@pbsds
Copy link
Contributor

pbsds commented Jan 14, 2024

Result of nixpkgs-review pr 274161 run on x86_64-linux 1

5 packages failed to build:
  • idutils
  • (succeeds on hydra)
  • rocmPackages.migraphx (rocmPackages_5.migraphx)
  • (fails on hydra)
  • rocmPackages.mivisionx (rocmPackages.mivisionx-hip ,rocmPackages_5.mivisionx ,rocmPackages_5.mivisionx-hip)
  • (fails on hydra)
  • rocmPackages.mivisionx-cpu (rocmPackages_5.mivisionx-cpu)
  • (fails on hydra)
  • rocmPackages.mivisionx-opencl (rocmPackages_5.mivisionx-opencl)
  • (fails on hydra)
27 packages built:
  • gnu-pw-mgr
  • gnulib
  • lbzip2
  • lua51Packages.lrexlib-gnu
  • luaPackages.lrexlib-gnu (lua52Packages.lrexlib-gnu)
  • lua53Packages.lrexlib-gnu
  • lua54Packages.lrexlib-gnu
  • luajitPackages.lrexlib-gnu
  • python310Packages.condaInstallHook
  • python310Packages.torchWithRocm
  • python310Packages.torchWithRocm.cxxdev
  • python310Packages.torchWithRocm.dev
  • python310Packages.torchWithRocm.dist
  • python310Packages.torchWithRocm.lib
  • python311Packages.condaInstallHook
  • python311Packages.torchWithRocm
  • python311Packages.torchWithRocm.cxxdev
  • python311Packages.torchWithRocm.dev
  • python311Packages.torchWithRocm.dist
  • python311Packages.torchWithRocm.lib
  • rocmPackages.miopen (rocmPackages.miopen-hip ,rocmPackages_5.miopen ,rocmPackages_5.miopen-hip)
  • rocmPackages.miopen-opencl (rocmPackages_5.miopen-opencl)
  • surf-display
  • wget2
  • wget2.dev
  • wget2.lib
  • xprintidle-ng

from idutils build log:

FAIL: test-update-copyright.sh

idutils seems to perform many gnulib tests, does it assume a specific/vendored gnulib @gfrascadorio?

EDIT: the tests seems to test build-aux/update-copyright, which now spits out the following warnings:

$ results/gnulib/build-aux/update-copyright
Use of uninitialized value $_ in pattern match (m//) at results/gnulib/build-aux/update-copyright line 171.
Use of uninitialized value $_ in pattern match (m//) at results/gnulib/build-aux/update-copyright line 177.
Use of uninitialized value $ARGV in concatenation (.) or string at results/gnulib/build-aux/update-copyright line 293.
: warning: copyright statement not found

@afh afh marked this pull request as draft June 1, 2024 15:57
@afh afh marked this pull request as ready for review June 1, 2024 18:16
@afh
Copy link
Member Author

afh commented Jun 2, 2024

Friendly ping to @anthonyroussel, @SuperSandro2000, and @pbsds on this to have another look at the changes proposed in this PR 🙂

@afh afh force-pushed the gnulib-subst-perl branch 2 times, most recently from d90301e to f00622e Compare June 3, 2024 07:29
@afh afh requested review from pbsds and SuperSandro2000 June 3, 2024 07:29
@afh
Copy link
Member Author

afh commented Jun 3, 2024

Thanks for the scrutinising review and and helpful comments, @pbsds and @SuperSandro2000, very much appreciated!

For good measure I took the liberty to use ${lib.getBin perl} instead of just ${perl} for the replacement, would you agree that that's a good call?

@pbsds
Copy link
Contributor

pbsds commented Jun 5, 2024

lib.getBin makes sense, but lib.getExe might be the better option.

Result of nixpkgs-review pr 274161 run on x86_64-linux 1

8 packages marked as broken and skipped:
  • rocmPackages.migraphx
  • rocmPackages.mivisionx
  • rocmPackages.mivisionx-cpu
  • rocmPackages.mivisionx-hip
  • rocmPackages_6.migraphx
  • rocmPackages_6.mivisionx
  • rocmPackages_6.mivisionx-cpu
  • rocmPackages_6.mivisionx-hip
14 packages failed to build:
  • idutils
  • python311Packages.torchWithRocm
  • python311Packages.torchWithRocm.cxxdev
  • python311Packages.torchWithRocm.dev
  • python311Packages.torchWithRocm.dist
  • python311Packages.torchWithRocm.lib
  • python312Packages.torchWithRocm
  • python312Packages.torchWithRocm.cxxdev
  • python312Packages.torchWithRocm.dev
  • python312Packages.torchWithRocm.dist
  • python312Packages.torchWithRocm.lib
  • rocmPackages_5.mivisionx (rocmPackages_5.mivisionx-hip)
  • rocmPackages_5.mivisionx-cpu
  • rocmPackages_5.mivisionx-opencl
20 packages built:
  • gnu-pw-mgr
  • gnulib
  • lbzip2
  • lua51Packages.lrexlib-gnu
  • luaPackages.lrexlib-gnu (lua52Packages.lrexlib-gnu)
  • lua53Packages.lrexlib-gnu
  • lua54Packages.lrexlib-gnu
  • luajitPackages.lrexlib-gnu
  • python311Packages.condaInstallHook
  • python312Packages.condaInstallHook
  • rocmPackages.miopen (rocmPackages.miopen-hip ,rocmPackages_6.miopen ,rocmPackages_6.miopen-hip)
  • rocmPackages_5.migraphx
  • rocmPackages_5.miopen (rocmPackages_5.miopen-hip)
  • rocmPackages_5.miopen-opencl
  • surf-display
  • wget2
  • wget2.dev
  • wget2.lib
  • xprintidle-ng
  • zluda

idutils still fails.

$ results/gnulib/build-aux/update-copyright
Use of uninitialized value $_ in pattern match (m//) at results/gnulib/build-aux/update-copyright line 171.
Use of uninitialized value $_ in pattern match (m//) at results/gnulib/build-aux/update-copyright line 177.
Use of uninitialized value $ARGV in concatenation (.) or string at results/gnulib/build-aux/update-copyright line 293.
: warning: copyright statement not found

the other failures also fail on hydra

@afh
Copy link
Member Author

afh commented Jun 5, 2024

Thanks, @pbsds, I'll look into it.

What's the difference between lib.getBin and lib.getExe (feel free to point me to relevant information should be explanation be too much effort).

@afh
Copy link
Member Author

afh commented Jun 5, 2024

One cause for the build failures could be that the update-copyright script is not properly invoked from idutils as it appears odd to me that $ARGV is empty as well as $_ the default input or search space variable… 🤔

Here are links to gnulib's update-copyright (respecting the src.rev used in nixpkgs):

@pbsds
Copy link
Contributor

pbsds commented Jun 5, 2024

It expands to ${lib.getBin drv}/bin/${drv.meta.mainProgram}

$ nix repl '<nixpkgs>'
Added 19880 variables.
nix-repl> lib.getBin hello
«derivation /nix/store/i6mny38n5j529m24sgnwl7jipishjmzb-hello-2.12.1.drv»
nix-repl> lib.getExe hello 
"/nix/store/fchfx8ykh12d0f8bq0yi3k80mcz7n250-hello-2.12.1/bin/hello"

I tried running the one currently in master, and it waits on stdin

$ $(nix build nixpkgs#gnulib --print-out-paths)/build-aux/update-copyright
-i used with no filenames on the command line, reading from STDIN.
^D
-: warning: copyright statement not found

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@afh
Copy link
Member Author

afh commented Jun 23, 2024

Result of nixpkgs-review pr 274161 run on aarch64-darwin 1

1 package marked as broken and skipped:
  • zluda
8 packages built:
  • gnulib
  • idutils
  • lbzip2
  • lua51Packages.lrexlib-gnu
  • luaPackages.lrexlib-gnu (lua52Packages.lrexlib-gnu)
  • lua53Packages.lrexlib-gnu
  • lua54Packages.lrexlib-gnu
  • luajitPackages.lrexlib-gnu

@afh
Copy link
Member Author

afh commented Jun 23, 2024

Apologies for the silence on this issue, folks. My attention was needed elsewhere. On the plus side: I now also have access to a x86_64-linux system for testing and can confirm that building the idutils package from this branch fails the update-copyright test.

What's curious is that idutils from this branch builds and passes the tests on aarch64-darwin. Any one have an idea what could cause the issue on linux?

@afh
Copy link
Member Author

afh commented Jun 23, 2024

Maybe it's alright to skip/disable the update-copyright test since:

It is important to have self tests for programs like that, but it's not as important that they run properly on non-development platforms.
https://lists.nongnu.org/archive/html/bug-gnulib/2011-11/msg00320.html

What do people think?

@afh
Copy link
Member Author

afh commented Jun 23, 2024

@pbsds I went ahead and disable the failing test on Linux. Would you have time and interest in having another look at giving it a try?

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4139

@ofborg ofborg bot requested a review from gfrascadorio June 23, 2024 20:34
@afh
Copy link
Member Author

afh commented Jun 27, 2024

Friendly ping to the current reviewers on this one 🙂
@anthonyroussel @pbsds @SuperSandro2000 @gfrascadorio

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

4 participants