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

command-not-found: make script portable #3914

Closed
wants to merge 1 commit into from

Conversation

ryuheechul
Copy link

Without /usr/bin/env -S, this breaks with bash on Darwin

Description

I discovered that shebang being a pure script (not binary) seems to be problematic with

bashInteractive package on Darwin (I imagine the same for
bash)

But works with zsh

So I found and tested the solution that works for both bash and zsh on Darwin as well as NixOS.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

    • not sure if any updates on test cases are required for this change
  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

Without `/usr/bin/env -S`, this breaks with bash on Darwin
@ryuheechul ryuheechul requested a review from rycee as a code owner April 24, 2023 15:52
@ncfavier
Copy link
Member

I guess that explains #3858.

This should probably be fixed in the spirit of NixOS/nixpkgs#124556 by using makeBinaryWrapper in https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/interpreters/perl/wrapper.nix.

@ryuheechul
Copy link
Author

@ncfavier Yeah, I think there is a high chance this would fix #3858 too.
Also as you suggested, fixing the perl wrapper seems to be better idea although I don't have a clue on how to do that.

@ncfavier
Copy link
Member

Replacing makeWrapper with makeBinaryWrapper in that file should take you most of the way there (will need testing, of course).

Comment on lines +1 to 3
#!/usr/bin/env -S @perl@/bin/perl -w

use strict;
Copy link
Member

Choose a reason for hiding this comment

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

Another alternative, that I think may be better:

Suggested change
#!/usr/bin/env -S @perl@/bin/perl -w
use strict;
#!@perl@/bin/perl
use strict;
use warnings;

@rycee
Copy link
Member

rycee commented Apr 24, 2023

Unfortunately, I think makeBinaryWrapper would probably not be the best for our use case since it would require most people to do a compile step.

Some time ago I worked on an alternative wrapper tool that attempts to be something of an intermediate between the standard Bash wrapper and the binary wrapper. That style of wrapper would be more suitable for our purposes, I think.

That said. I think in this case the best solution would be use remove the -w option and use use warnings.

@ncfavier
Copy link
Member

it would require most people to do a compile step

What's wrong with that? Compiling the wrapper is very fast.

remove the -w option

I don't see how this fixes the issue at hand, which is that Darwin doesn't support recursive shebangs, and @perl@/bin/perl is a shebang script.

Svep is a neat idea though, short of proper declarative wrappers...

@ryuheechul
Copy link
Author

@ncfavier I see. Hopefully that can be addressed at upstream but I don't think I'm stepping into it today though.

Also I just learned that even if this shebang thing is fixed, the experience with command-not-found on darwin is just broken as adding nixos channel to root doesn't actually work properly.

However, I also learned that using nix-index instead works similar to the experience of command-not-found on nixos. So I settled with my workaround like this one, ryuheechul/dotfiles@333f22e.

So I'm closing this PR for now.

Thanks for both @ncfavier and @rycee, I learned more about the subject thanks to your comments :)

@ryuheechul ryuheechul closed this Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants