shebang: raise error if no rewriting #12820

branchvincent · 2022-02-01T05:20:28Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

In Homebrew/homebrew-core#94202, we encountered a bug where rewrite_shebang wasn't actually rewriting any shebang due to a misspelled filename. This change prevents this class of bugs by erroring when no replacement occurs.

This is my first PR here, so any feedback is greatly appreciated 😄

BrewTestBot · 2022-02-01T05:20:44Z

Review period will end on 2022-02-02 at 05:20:28 UTC.

MikeMcQuaid · 2022-02-01T08:37:07Z

Makes sense to me, thanks @branchvincent!

BrewTestBot · 2022-02-01T08:37:12Z

Review period skipped due to critical label.

Rylan12

Thanks, @branchvincent! Looks good to me too!

iMichka · 2022-02-01T18:15:04Z

I nominate this PR for the "best Homebrew feature of February 2022 award" :)
Thanks!

branchvincent · 2022-02-01T22:30:42Z

thanks everyone!

meissnem · 2022-02-02T05:03:36Z

Perhaps I'm doing something wrong, but after this PR hit, a brew install -s postgresql looks like it succeeds (with a new warning) but the return code is 1.

joshgoebel · 2022-02-02T06:03:38Z

For anyone getting the error:

RuntimeError (No matching files found to rewrite shebang)

You can fix this by locally by temporarily reverting this commit: 4c65c27cff75a92d2a20a1f56e51f9f91f4da4e1 #12826

branchvincent · 2022-02-02T13:56:39Z

@meissnem @joshgoebel thanks for the heads up, this was fixed by reverting in #12828. sorry for the breakage

Rylan12 · 2022-02-02T17:10:09Z

I'm still not entirely sure this is PR is wrong. It seems reasonable to have an error when there's no replacement, although maybe a warning is a better option so it doesn't break things.

How often do we use a single rewrite_shebang vs multiple (i.e. in a loop of some sort)? If it's pretty even, I wonder if we want to have rewrite_shebang error if nothing was done, and add a new rewrite_shebangs method that takes either a list of files, a directory, or a wildcard string path to expand (etc.) and errors if none of the paths passed have a replacement.

If that's not worth the effort, we can use RuboCop to ensure that you don't pass a wildcard path (e.g. *.py) to rewrite_shebang in an attempt to solve the specific problem that prompted this PR.

I guess another option is Homebrew/homebrew-core#94328 — just avoid putting rewrite_shebang in this kind of situation. This would also have a rubocop, ideally

meissnem · 2022-02-02T17:16:27Z

I'm just a user and formula-dabbler with homebrew so I might be way off base -- but the postgresql formula is not calling rewrite_shebang directly. I think it's being called indirectly through formula_installer.rb which in turn calls into Cleaner.clean ~~which calls rewrite-shebangs with no arguments~~ I missed a level of indirection 🙂 Cleaner.clean calls Cleaner.rewrite_shebangs which calls Utils::Shebang.rewrite_shebang.

Rylan12 · 2022-02-02T17:20:46Z

@meissnem what was the text of the warning you received when you tried to install?

meissnem · 2022-02-02T17:24:19Z

Warning: The cleaning step did not complete successfully
Still, the installation was successful, so we will link it into your prefix.
==> No matching files found to rewrite shebang
/usr/local/Homebrew/Library/Homebrew/utils/shebang.rb:47:in `rewrite_shebang'
/usr/local/Homebrew/Library/Homebrew/cleaner.rb:137:in `block in rewrite_shebangs'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/pathname.rb:565:in `block in find'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/find.rb:49:in `block (2 levels) in find'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/find.rb:48:in `catch'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/find.rb:48:in `block in find'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/find.rb:43:in `each'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/find.rb:43:in `find'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/pathname.rb:565:in `find'
/usr/local/Homebrew/Library/Homebrew/cleaner.rb:131:in `rewrite_shebangs'
/usr/local/Homebrew/Library/Homebrew/cleaner.rb:37:in `clean'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:1088:in `clean'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:456:in `install'
/usr/local/Homebrew/Library/Homebrew/upgrade.rb:208:in `install_formula'
/usr/local/Homebrew/Library/Homebrew/install.rb:327:in `install_formula'
/usr/local/Homebrew/Library/Homebrew/install.rb:317:in `block in install_formulae'
/usr/local/Homebrew/Library/Homebrew/install.rb:316:in `each'
/usr/local/Homebrew/Library/Homebrew/install.rb:316:in `install_formulae'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:218:in `install'
/usr/local/Homebrew/Library/Homebrew/brew.rb:110:in `<main>'

But as I said, the return code was 1 which caused problems.

As a work-around, I added everything that is installed to skip_clean and that made the warning go away and the return code 0, but then lost the benefit of the clean step fixing permissions.

Rylan12 · 2022-02-02T17:29:52Z

Gotcha. I think that would be handled by raising a ShebangDetectionError instead for the error message (like this).

joshgoebel · 2022-02-02T17:39:22Z

I'm still not entirely sure this is PR is wrong. It seems reasonable to have an error when there's no replacement, although maybe a warning is a better option so it doesn't break things.

Agreed on this point. The PR was trying to do a good thing (report errors with re-writing), just didn't take these loop cases into account.

errors if none of the paths passed have a replacement.

I think there is a potential issue with wildcards in general... but I'm not sure how you fix it... being more general can make the formulas more resilient. For example consider something like: rewrite *.py against a directory of scripts...

This works (if you add more scripts)... until someone adds a Python script with no extension .py... in which case now it's broken with no error reported... hard-coding all the script names would solve this, but then potentially be a burden for maintainers when a new script is added. I'm of course assuming here there is some good reason we need all these rewritten vs just relying on env/path behavior...

rewrite bin/* and making sure "at least one" in touched might be as good as it gets though.

carlocab · 2022-02-02T22:58:53Z

Personally, I'd opt for a simpler approach: add an argument to rewrite_shebang that enables/disables the behaviour added in this PR (a little like the audit_result argument in inreplace), with the check from this PR enabled by default.

We can then disable the check for any rewrite_shebang call inside brew itself and optionally add an audit that prevents disabling the check inside formulae.

Either that or we fix all the rewrite_shebang calls inside brew (if that's possible; I don't see right now why it shouldn't be) and then revert the revert of this PR.

Rylan12 · 2022-02-03T05:11:20Z

If it doesn't break backward compatibility too much, I'd like to see the error message shown by default but I won't complain if we need to do it the other way.

carlocab · 2022-02-03T05:26:36Z

Yea, my idea was for rewrite_shebang to error out by default. We would then disable the error from calls to it inside brew and outside a formula.

Alternatively, we could probably just check for a shebang here before attempting to rewrite it:

brew/Library/Homebrew/cleaner.rb

Lines 130 to 141 in 8e97f60

    
           basepath = @f.prefix.realpath 
        
           basepath.find do |path| 
        
             Find.prune if @f.skip_clean? path 
        
             next if path.directory? || path.symlink? 
        
             begin 
        
               Utils::Shebang.rewrite_shebang Language::Perl::Shebang.detected_perl_shebang(@f), path 
        
             rescue ShebangDetectionError 
        
               break 
        
             end 
        
           end

Reading the first two bytes should be enough.

No strong preference between the two, although I suspect avoiding the file system when it isn't really needed is a good idea.

Rylan12 · 2022-02-03T05:34:34Z

As it's currently written we shouldn't really need to do anything there. rewrite_shebang already checks for a shebang before attempting to replace it, so it feels weird to check that again here. The problem in that particular place is that since found (in the proposed and since reverted rewrite_shebang method) isn't set to true until after the replacement, so in those cases where there isn't a shebang, no files are found because to be found doesn't just mean "be a file" but "be a file with a shebang that was rewritten".

carlocab · 2022-02-03T05:55:28Z

Library/Homebrew/utils/shebang.rb

+      found = T.let(false, T::Boolean)
      paths.each do |f|
        f = Pathname(f)
        next unless f.file?
        next unless rewrite_info.regex.match?(f.read(rewrite_info.max_length))

        Utils::Inreplace.inreplace f.to_s, rewrite_info.regex, "#!#{rewrite_info.replacement}"
+        found = true
      end
+
+      raise "No matching files found to rewrite shebang" unless found


Suggested change

found = T.let(false, T::Boolean)

paths.each do |f|

f = Pathname(f)

next unless f.file?

next unless rewrite_info.regex.match?(f.read(rewrite_info.max_length))

Utils::Inreplace.inreplace f.to_s, rewrite_info.regex, "#!#{rewrite_info.replacement}"

found = true

end

raise "No matching files found to rewrite shebang" unless found

found = paths.all? do |f|

f = Pathname(f)

next false unless f.file?

next true unless rewrite_info.regex.match?(f.read(rewrite_info.max_length))

Utils::Inreplace.inreplace f.to_s, rewrite_info.regex, "#!#{rewrite_info.replacement}"

true

end

raise "No matching files found to rewrite shebang" unless found

@Rylan12 maybe something like this then?

#all? probably doesn't work though, since I suspect it is greedy (bails out of the iterator as soon as it sees false)

Yeah, that looks like it would work (I think you're right about #all? though so we might have to write that ourselves).

We also might want to change the error message since it's not necessarily that no files were found just that one file wasn't.

shebang: raise error if no rewriting

7e6be5e

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 1, 2022

branchvincent requested a review from Rylan12 February 1, 2022 05:26

MikeMcQuaid approved these changes Feb 1, 2022

View reviewed changes

MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Feb 1, 2022

MikeMcQuaid enabled auto-merge February 1, 2022 08:37

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 1, 2022

BrewTestBot approved these changes Feb 1, 2022

View reviewed changes

MikeMcQuaid merged commit 955292d into Homebrew:master Feb 1, 2022

branchvincent deleted the shebang branch February 1, 2022 13:49

Rylan12 reviewed Feb 1, 2022

View reviewed changes

This was referenced Feb 2, 2022

apt 2.3.15 Homebrew/homebrew-core#94323

Closed

samba 4.15.5 Homebrew/homebrew-core#94321

Closed

branchvincent mentioned this pull request Feb 2, 2022

misc: prevent rewrite_shebang from erroring Homebrew/homebrew-core#94328

Closed

6 tasks

carlocab reviewed Feb 3, 2022

View reviewed changes

github-actions bot added the outdated PR was locked due to age label Mar 6, 2022

github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shebang: raise error if no rewriting #12820

shebang: raise error if no rewriting #12820

branchvincent commented Feb 1, 2022

BrewTestBot commented Feb 1, 2022

MikeMcQuaid commented Feb 1, 2022

BrewTestBot commented Feb 1, 2022

Rylan12 left a comment

iMichka commented Feb 1, 2022

branchvincent commented Feb 1, 2022

meissnem commented Feb 2, 2022

joshgoebel commented Feb 2, 2022 •

edited

branchvincent commented Feb 2, 2022

Rylan12 commented Feb 2, 2022

meissnem commented Feb 2, 2022 •

edited

Rylan12 commented Feb 2, 2022

meissnem commented Feb 2, 2022

Rylan12 commented Feb 2, 2022

joshgoebel commented Feb 2, 2022 •

edited

carlocab commented Feb 2, 2022

Rylan12 commented Feb 3, 2022

carlocab commented Feb 3, 2022

Rylan12 commented Feb 3, 2022

carlocab Feb 3, 2022

Rylan12 Feb 3, 2022

shebang: raise error if no rewriting #12820

shebang: raise error if no rewriting #12820

Conversation

branchvincent commented Feb 1, 2022

BrewTestBot commented Feb 1, 2022

MikeMcQuaid commented Feb 1, 2022

BrewTestBot commented Feb 1, 2022

Rylan12 left a comment

Choose a reason for hiding this comment

iMichka commented Feb 1, 2022

branchvincent commented Feb 1, 2022

meissnem commented Feb 2, 2022

joshgoebel commented Feb 2, 2022 • edited

branchvincent commented Feb 2, 2022

Rylan12 commented Feb 2, 2022

meissnem commented Feb 2, 2022 • edited

Rylan12 commented Feb 2, 2022

meissnem commented Feb 2, 2022

Rylan12 commented Feb 2, 2022

joshgoebel commented Feb 2, 2022 • edited

carlocab commented Feb 2, 2022

Rylan12 commented Feb 3, 2022

carlocab commented Feb 3, 2022

Rylan12 commented Feb 3, 2022

carlocab Feb 3, 2022

Choose a reason for hiding this comment

Rylan12 Feb 3, 2022

Choose a reason for hiding this comment

joshgoebel commented Feb 2, 2022 •

edited

meissnem commented Feb 2, 2022 •

edited

joshgoebel commented Feb 2, 2022 •

edited