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

x3270 4.0ga10 #58711

Closed
wants to merge 1 commit into from
Closed

x3270 4.0ga10 #58711

wants to merge 1 commit into from

Conversation

chimurai
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@chimurai
Copy link
Contributor Author

chimurai commented Jul 27, 2020

Failed on:

stable version should not decrease (from 4.0ga9 to 4.0ga10)

Also tried with brew bump-formula-pr and it's returning the same error message.

Not sure how to fix this since the versioning scheme is not following semver.

@chimurai chimurai closed this Jul 27, 2020
@chimurai chimurai reopened this Jul 27, 2020
@gromgit
Copy link
Member

gromgit commented Jul 28, 2020

Since all released versions seem to have the same scheme, I think the way forward is to manually specify the version, so you need to add this to the formula:

version "4.0.10"
version_scheme 1

Please read https://rubydoc.brew.sh/Formula.html, in particular the class attributes version and version_scheme.

@chimurai
Copy link
Contributor Author

Thanks @gromgit for the suggestion. I didn't know that.

Updated the PR.

@SMillerDev
Copy link
Member

I don't think we should be using a different version than upstream unless they specifically mention that x.x.x is the version scheme and x.xgax is just for the archives

@chimurai
Copy link
Contributor Author

chimurai commented Jul 28, 2020

When I print the version with c3270 --version, this is the output:

c3270 v4.0ga10

@SMillerDev do you have a suggestion how to proceed?

@SMillerDev
Copy link
Member

You should define the version as upstream defines it. Don't define it as semver. And try without version scheme if possible.

@gromgit
Copy link
Member

gromgit commented Jul 28, 2020

Since the CI is already complaining because "4.0ga10" < "4.0ga9", isn't at least a version_scheme 1 declaration necessary?

@chimurai
Copy link
Contributor Author

Getting same error message with or without version_scheme, when defining the same version as defined by x3270.

With just version (without version_scheme):
version "4.0ga10"

After brew audit --strict x3270

  • Stable: version 4.0ga10 is redundant with version scanned from URL

With version including version_scheme:
version "4.0ga10"
version_scheme 1

After brew audit --strict x3270

  • Stable: version 4.0ga10 is redundant with version scanned from URL

@SMillerDev
Copy link
Member

In that case you don't need to define the version line. I think this might be a @Homebrew/brew bug in that case though.

@MikeMcQuaid
Copy link
Member

I don't think it's a bug. The version being redundant is unrelated to the version_scheme.

@gromgit
Copy link
Member

gromgit commented Jul 28, 2020

@chimurai , just add version_scheme 1 without the version line. That should tell Homebrew to "reset" its idea of the base version.

@MikeMcQuaid
Copy link
Member

Since the CI is already complaining because "4.0ga10" < "4.0ga9", isn't at least a version_scheme 1 declaration necessary?

This is the bit that should be fixed in Homebrew/brew.

@SMillerDev
Copy link
Member

What about it thinking the version is lower though @MikeMcQuaid ?

@MikeMcQuaid
Copy link
Member

What about it thinking the version is lower though @MikeMcQuaid ?

Yeh, I'd consider that a bug worth fixing in Homebrew/brew.

@chimurai
Copy link
Contributor Author

@gromgit adding just version_scheme 1 works. All checks passed.

Should this PR wait for the bugfix in Homebrew/brew?

@samford
Copy link
Member

samford commented Jul 28, 2020

Yeh, I'd consider that a bug worth fixing in Homebrew/brew.

I've drafted a fix for the version comparison issue in Homebrew/brew#8125, for what it's worth.

@gromgit
Copy link
Member

gromgit commented Jul 29, 2020

@gromgit adding just version_scheme 1 works. All checks passed.

Should this PR wait for the bugfix in Homebrew/brew?

Yes, please wait for the outcome of @samford 's patch. If it goes through, then remove the version_scheme line and push it again.

@samford
Copy link
Member

samford commented Jul 29, 2020

Homebrew/brew#8125 is merged, so the version_scheme line can be removed now.

@MikeMcQuaid
Copy link
Member

But note this PR cannot be merged until that's in a tagged release.

@chimurai
Copy link
Contributor Author

Removed version_scheme from the PR.

@BrewTestBot BrewTestBot added the missing license Formula has a missing license which should be added label Jul 31, 2020
@dawidd6 dawidd6 removed the missing license Formula has a missing license which should be added label Jul 31, 2020
@BrewTestBot BrewTestBot added the missing license Formula has a missing license which should be added label Aug 4, 2020
@jonchang
Copy link
Contributor

jonchang commented Aug 5, 2020

License here is BSD-3-clause: http://x3270.bgp.nu/license.html

@chimurai
Copy link
Contributor Author

chimurai commented Aug 7, 2020

Added license "BSD-3-Clause" to this PR

@samford samford removed the missing license Formula has a missing license which should be added label Aug 10, 2020
Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

A new Homebrew/brew version has been tagged (2.4.10), so we can merge this now. I rebased this and CI looks good.

Thanks, @chimurai!

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@chimurai
Copy link
Contributor Author

Thanks for the amazing support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants