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

audit: fix "version should not decrease" check. #1493

Merged
merged 1 commit into from
Nov 13, 2016
Merged

audit: fix "version should not decrease" check. #1493

merged 1 commit into from
Nov 13, 2016

Conversation

MikeMcQuaid
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 tests with your changes locally?

Fix the "version should not decrease" check so it correctly handlesversion_schemes.

CC @ilovezfs @zmwangx

Fixes #1489.

@MikeMcQuaid MikeMcQuaid added the in progress Maintainers are working on this label Nov 13, 2016
@ilovezfs
Copy link
Contributor

Library/Homebrew/formula_versions.rb:78:11: C: Use next to skip iteration.

@ilovezfs
Copy link
Contributor

Crash bug with dbus:

iMac-TMP:~ joe$ brew audit dbus
dbus:
  * version should not decrease
Error: 1 problem in 1 formula
iMac-TMP:~ joe$ brew pull https://github.com/Homebrew/brew/pull/1493
==> Fetching patch 
Patch: https://github.com/Homebrew/brew/pull/1493.patch
==> Applying patch
Applying: audit: fix "version should not decrease" check.
==> Patch closes issue #1493
==> Patch changed:
 Library/Homebrew/dev-cmd/audit.rb    | 37 ++++++++++++++++++++++++------------
 Library/Homebrew/formula_versions.rb | 16 +++++++++++++---
 2 files changed, 38 insertions(+), 15 deletions(-)
iMac-TMP:~ joe$ brew audit dbus
Error: undefined method `empty?' for nil:NilClass
Please report this bug:
  https://git.io/brew-troubleshooting
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:675:in `block (2 levels) in audit_revision_and_version_scheme'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:670:in `each'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:670:in `block in audit_revision_and_version_scheme'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:669:in `each'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:669:in `audit_revision_and_version_scheme'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:1065:in `audit'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:72:in `block in audit'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:68:in `each'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:68:in `audit'
/usr/local/Homebrew/Library/Homebrew/brew.rb:94:in `<main>'

Fix the "version should not decrease" check so it correctly handles
`version_scheme`s.

Fixes #1489.
@MikeMcQuaid
Copy link
Member Author

Fixed those issues but dbus will still fail because we've got a version number decrease without a version_scheme bump. Need to think how best to address that but probably worth merging this as-is for now as it's not any worse.

@ilovezfs
Copy link
Contributor

Maybe not worse, but it did change the output

iMac-TMP:Homebrew joe$ brew audit dbus
dbus:
  * stable version should not decrease
Error: 1 problem in 1 formula

@MikeMcQuaid
Copy link
Member Author

Yeh, it adds "stable" on to be clear which spec it's referring to. I think that's probably better in figuring out what's happened.

@ilovezfs
Copy link
Contributor

Should this really close #1489 if the dbus problem is still unfixed?

next if spec_attribute_map.nil? || spec_attribute_map.empty?

attributes_for_version = spec_attribute_map[formula.version]
next if attributes_for_version.nil? || attributes_for_version.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

any way to avoid the repetitiveness of 671-672 and 674-675?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, nope.

end

return if formula.revision.zero?
if formula.stable
revision_map = attributes_map[:revision]
revision_map = attributes_map[:revision][:stable]
Copy link
Contributor

Choose a reason for hiding this comment

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

can attributes_map[:revision] be nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

No.

@MikeMcQuaid
Copy link
Member Author

Should this really close #1489 if the dbus problem is still unfixed?

Yes because the issue described in the title/body has been addressed: this is now considering version_scheme. It's a different problem to figure out how we do this with "latest" vs. "compare the last 10 formula changes".

@MikeMcQuaid
Copy link
Member Author

Arguably we should consider bumping version_scheme whenever we accidentally pull a too-new version and revert back to a stable one. The audit check is correct in this case, it's just not how we want it to behave.

@MikeMcQuaid MikeMcQuaid merged commit fc3d586 into Homebrew:master Nov 13, 2016
@MikeMcQuaid MikeMcQuaid deleted the audit-version-decrease-fix branch November 13, 2016 14:06
@MikeMcQuaid MikeMcQuaid removed the in progress Maintainers are working on this label Nov 13, 2016
@MikeMcQuaid
Copy link
Member Author

To be clear though: I'm definitely 👍 on having a new issue describing how we do want this to behave in the dbus case.

@ilovezfs
Copy link
Contributor

Since version_scheme didn't exist then, and the too new one was rolled back minutes later, it seems goofy to have to slap a version scheme on it at this point, and an abuse of what that DSL was for.

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 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 this pull request may close these issues.

2 participants