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

extract: use downcased version in filename #11440

Merged
merged 1 commit into from
May 27, 2021

Conversation

scpeters
Copy link
Member

  • 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?

Formula filenames are required to be lowercase, so the brew extract command should downcase the version before using it to construct the filename. I noticed this while performing brew extract tbb@2020, which has version 2020_U3 and resulted in a formula with filename tbb@2020_U3.rb

Formula filenames are required to be lowercase, so the
extract command should downcase version before
using it to construct the filename. I noticed this while
extracting tbb@2020, which has version 2020_U3
and resulted in tbb@2020_U3.rb
@BrewTestBot
Copy link
Member

Review period will end on 2021-05-27 at 06:33:02 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label May 26, 2021
@scpeters
Copy link
Member Author

I suppose that we may also want to add a diagnostic to complain about capital letters in formula filenames, since it took me a little while to figure out what the problem was that led to this fix

@Bo98
Copy link
Member

Bo98 commented May 27, 2021

it took me a little while to figure out what the problem was that led to this fix

What were you seeing when it had an uppercase character? An error?

@scpeters
Copy link
Member Author

it took me a little while to figure out what the problem was that led to this fix

What were you seeing when it had an uppercase character? An error?

I was trying to build bottles for tbb@2020_U3 and a dependent in the same pull request (osrf/homebrew-simulation#1483). The tbb@2020_U3 bottle built fine, but when installing dependencies for the dependent formula, it was trying to fetch the bottle that it had just built, even though it was still installed:

==> Running Formulae#formula!(osrf/simulation/gazebo11)
==> Determining dependencies...
==> brew fetch --retry ampl-mp aom ... tbb tbb@2020_U3 ... zimg
==> FAILED
...
==> Fetching tbb@2020_U3 from osrf/simulation
==> Downloading https://osrf-distributions.s3.amazonaws.com/bottles-simulation/tbb%402020_U3-2020_U3.catalina.bottle.tar.gz
curl: (22) The requested URL returned error: 404 Not Found
==> Retrying download
==> Downloading https://osrf-distributions.s3.amazonaws.com/bottles-simulation/tbb%402020_U3-2020_U3.catalina.bottle.tar.gz
curl: (22) The requested URL returned error: 404 Not Found
Error: Failed to download resource "tbb@2020_U3"
Download failed: https://osrf-distributions.s3.amazonaws.com/bottles-simulation/tbb%402020_U3-2020_U3.catalina.bottle.tar.gz

I couldn't pinpoint the exact problem, but it may be related to a downcase call in formulary.rb. I also noticed in the formula cookbook that "Filenames should be all lowercase".

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label May 27, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@Bo98
Copy link
Member

Bo98 commented May 27, 2021

Hmm yeah it should probably handled a bit better. This fix is correct to make anyway.

@Bo98 Bo98 merged commit 7ae9cc3 into Homebrew:master May 27, 2021
@scpeters scpeters deleted the extract_downcased_version branch May 27, 2021 16:10
@scpeters
Copy link
Member Author

I suppose that we may also want to add a diagnostic to complain about capital letters in formula filenames

#11447

@MikeMcQuaid
Copy link
Member

Nice work here @scpeters!

@github-actions github-actions bot added the outdated PR was locked due to age label Jul 1, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants