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

allowing sha256 :no_check to override version '<string>' #6356

Closed
rolandwalker opened this issue Sep 27, 2014 · 7 comments · Fixed by #8190
Closed

allowing sha256 :no_check to override version '<string>' #6356

rolandwalker opened this issue Sep 27, 2014 · 7 comments · Fixed by #8190
Assignees

Comments

@rolandwalker
Copy link
Contributor

Pulling this issue in to the main repo from Homebrew/homebrew-cask-versions#359, and pinging @caskroom/maintainers.

Proposal

sha256 :no_check would solely define whether or not checksums get checked.

Purpose

To keep and use version information which is necessary for brew cask upgrade.

Example

This would become a valid combination, and the checksum would be ignored:

version '1.2'
sha256 :no_check

Detail

version :latest + sha256 :no_check would still be legal.

version :latest + sha256 '<actual_sha_value>' would still be illegal.

Discussion

@vitorgalvao, it seems you already disagree here, but I would like to convince you — because I myself can't design a cleaner solution, and would like to move forward on implementation. Here is my line of reasoning:

  • For brew cask upgrade to work, the system simply has to track version numbers, somewhere, somehow. If brew cask upgrade cannot look up a version number, it cannot know when an upgrade is needed.
  • We have appcast to finesse that in certain cases, but there are other cases where appcast is not and will never be available. That is true for some apps, and for every non-app Cask such as a font or binary.
  • We already have a version stanza, so that is the logical place to put the version data.
  • We could create a second version stanza, called version_as_used_by_upgrade or whatever. That would allow us to leave version exactly as it is now — but for a human reading/authoring Casks, it would be tremendously confusing to have two competing version stanzas.
  • This is a very human-oriented project.
  • ":no_check means what it says" is a much simpler rule for a human.
  • We are currently throwing away perfectly good information by omitting the last-seen version when the version is known.
@vitorgalvao
Copy link
Member

Short answer is I’m not very opposed to it, and I certainly get your points. If it’s the best solution we have (and it seems like it might be), then lets do it, as the benefits far outweigh the costs.

Long answer is:

For brew cask upgrade to work, the system simply has to track version numbers

We’ll eventually retire :latest, right?

it would be tremendously confusing to have two competing version stanzas

Regarding this point, I agree; I don’t think creating another closely related stanza would be the solution.

I’d just like to touch on the following point:

":no_check means what it says" is a much simpler rule for a human

Although it’s true that it’s simple (and clear) when reading, my argument was that it’s confusing when writing, since many first-time contributors don’t initially understand that getting the shasum is something they, as the downloader of the software, can do. The check is a way to not erroneously merge those cases (which used to happen sporadically).

Can’t we make travis “pass the build with warnings” instead of breaking, for certain cases? We could then add this case there, which would call attention to it, and could even include the whitespace cases you’ve been solving but expressed a wish to not directly include as a breaking requirement.

@rolandwalker
Copy link
Contributor Author

My prediction is that we will never quite retire :latest. I'm willing to try, but there are a lot of corner cases out there.

Travis doesn't support "pass with warnings" exactly, but there is a way we could hack that, as a separate build that doesn't affect the badge. The reviewer would have to click on the Travis result every time to see it. The other thing I'd like Travis to do is apply additional tests to only the Casks changed by the PR (download, checksum, even install/uninstall). That would require trickery, but it may be possible. @federicobond, did you do an experiment with that?

I'm glad we can agree/compromise on the fundamentals here. I'll make Travis changes or whatever else we can think of to assist in processing submissions.

@tapeinosyne
Copy link
Contributor

There is another case where including version information with :no_check would be helpful: packages with versioned URLs which are nonetheless updated in place (e.g. for minor/patch versions). For example:

We are currently unable to reconcile versioning with unreliable URLs, thus such casks always break.

@federicobond
Copy link
Contributor

I must have missed this discussion. Yes, I did some tests on Travis for the use cases you are describing, and it is definitely possible to perform additional testing on the updated casks. I'm a bit worried about stepping outside of the fair use guidelines for Travis due to increased bandwidth usage though.

@tapeinosyne
Copy link
Contributor

I would suggest to take a decision on this before cutting the next release.

@vitorgalvao
Copy link
Member

I say we do it, but request a comment to that effect. There are already cases where a comment is required for clarity (i.e. apps that use :target, or where a download url is in a different host from the homepage), so we could do it here as well.

version '1.2.3'
sha256 :no_check # required as upstream package is updated in-place

This way we have a low friction solution that (hopefully) makes it clear in which cases it is acceptable to use this combination, and hopefully thwart mistakes/misunderstandings in submissions without breaking the build (if the comment is there, it’s more likely the contributor understands when it should or not be used).

#7062 will become even more important, following this route.

@tapeinosyne
Copy link
Contributor

I say we do it, but request a comment to that effect. There are already cases where a comment is required for clarity […], so we could do it here as well.

Yes, I agree.

@rolandwalker rolandwalker self-assigned this Nov 14, 2014
rolandwalker added a commit to rolandwalker/homebrew-cask that referenced this issue Dec 17, 2014
This behavior was traditionally present, and is now recovered
by removing the audit test added in Homebrew#4743.  The doc is clarified
but did not require major change.

closes: Homebrew#6356
refs: Homebrew#8179
vitorgalvao added a commit that referenced this issue Dec 17, 2014
[HOLD until #6356] :no_check in versioned casks
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants