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

github_packages: fix versioned bottle names. #11048

Merged

Conversation

MikeMcQuaid
Copy link
Member

@ cannot be used in Docker image names. Use AT instead (which we already use in class names so has some precedent).

Make mktemp consistent, too.

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Apr 6, 2021
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

BrewTestBot
BrewTestBot previously approved these changes Apr 6, 2021
Copy link
Member

@sjackman sjackman left a comment

Choose a reason for hiding this comment

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

Name components may contain lowercase letters, digits and separators. A separator is defined as a period, one or two underscores, or one or more dashes. A name component may not start or end with a separator.

https://docs.docker.com/engine/reference/commandline/tag/

Upper case letters are not permitted in name components. Good idea otherwise!

@sjackman
Copy link
Member

sjackman commented Apr 6, 2021

How about a single hyphen or underscore? gcc-10 has a nice ring to it.

$ ls *-[0-9]*.rb
ntfs-3g.rb pyqt-3d.rb
$ ls *-[0-9].rb
zsh: no matches found: *-[0-9].rb

I am actually surprised that no formulae match .*-[0-9].rb

@mistydemeo
Copy link
Member

That still leaves slight ambiguity in parsing, but if we don't have any formulae that meet that pattern we're probably safe. Doesn't look like there are any third-party tap packages in formulae.brew.sh that suggests we'll probably be fine. We might want to think about writing an audit just for the main tap to ensure we don't accidentally commit formulae that would clash with another one?

@MikeMcQuaid
Copy link
Member Author

Like @mistydemeo I’d rather figure out something unambiguous in parsing. We already use single dash, double dash, single underscore, single period as separators. We don’t use double underscores so I’d propose one of those.

Another slightly off the wall option that would be permitted would be gcc/10. Definitely no risk of clashing in that case and given we’re already doing core/gcc then core/gcc/10 feels like it makes sense. The more I think about this the more I like it. Thoughts?

BrewTestBot
BrewTestBot previously approved these changes Apr 6, 2021
BrewTestBot
BrewTestBot previously approved these changes Apr 6, 2021
BrewTestBot
BrewTestBot previously approved these changes Apr 6, 2021
`@` cannot be used in Docker image names. Use `/` instead (which we
already use in image names so has some precedent).

Make `mktemp` use `AT` (consistent with `Formula` subclasses), too.
@MikeMcQuaid
Copy link
Member Author

@sjackman 👍🏻 d in Slack.

@mistydemeo
Copy link
Member

I feel a little weird about / just because it looks like the org/repo divider in fully-qualified formula names. Not actually ambiguous in this context, just looks kind of weird. I'd slightly prefer a double underscore but I don't think it's too critical.

@MikeMcQuaid MikeMcQuaid merged commit 6d0275f into Homebrew:master Apr 6, 2021
@MikeMcQuaid MikeMcQuaid deleted the github_packages_versioned_bottles branch April 6, 2021 18:42
@github-actions github-actions bot added the outdated PR was locked due to age label May 7, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants