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

crystal 0.26.0 #31086

Closed

Conversation

matiasgarciaisaia
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?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

We've officially released 0.26.0 some hours ago, so here we are once again :)

Thanks, as always!

@DomT4
Copy link
Member

DomT4 commented Aug 14, 2018

Does this remain incompatible with llvm 6.0.1, out of interest?

@matiasgarciaisaia
Copy link
Contributor Author

The builds are failing, waiting on amberframework/amber#922 (and a subsequent upgrade in its formula?)

I'm not sure if this is the place, but we should discuss how will Crystal and its ecosystem fit within Homebrew before hitting 1.0.

Is Homebrew OK with waiting until amber gets updated to have crystal 0.26.0? In that case, we may have a Crystal-maintained tap that gets releases as soon as they're out, and then let Homebrew work its times.

Making Crystal releases depend on the rest of the ecosystem doesn't feel that good.

Once we hit 1.0 everything should be fine - dependent formulae won't break upon minor releases, so there won't be major issues then. But in the meantime, I'm not sure about the best way to go.

@bcardiff
Copy link
Contributor

@DomT4 AFAIK llvm 6 should be supported in this version. There has been some PR for its support, yet we don't have CI for it yet.

Maybe the llvm@5 can be relaxed.

@jkthorne
Copy link

@bcardiff I can install with the default "llvm" package from homebrew.

@felixbuenemann
Copy link
Contributor

I've been running Crystal 0.25.1 with the patches from crystal-lang/crystal#6380 for some time with no problems, so I think changing the dependency from llvm@5 to llvm should be fine.

@DomT4
Copy link
Member

DomT4 commented Aug 14, 2018

(and a subsequent upgrade in its formula?)
Is Homebrew OK with waiting until amber gets updated to have crystal 0.26.0?

If there's no imminent release planned upstream there we could apply a short-term patch, if necessary, and bump the revision. A formal release with compatibility would be ideal, but if compatibility is there but simply not in a release yet and we can patch in that compatibility with patch do blobs, I can be okay with that short-term.

so I think changing the dependency from llvm@5 to llvm should be fine.

Sounds good!

@Willamin
Copy link

Willamin commented Aug 14, 2018

Making Crystal releases depend on the rest of the ecosystem doesn't feel that good.

Totally agreed. Not everyone uses Amber. In my opinion, if someone's project requires an old version of a language, they should be using a version manager to ensure compatibility. There is an asdf plugin for Crystal over at https://github.com/marciogm/asdf-crystal

@Willamin
Copy link

Willamin commented Aug 14, 2018

Is there any chance we could get a home-brew option for something like --latest to grab the latest released Crystal version regardless of the compatibility of other packages like Amber? There's already a --HEAD option, so it seems like there's a bit of a precedent set.

Or maybe that's not quite possible with the way homebrew formulae are written.

@felixbuenemann
Copy link
Contributor

Well it would be possible to add a devel block and bump it as soon as a new release is tagged, you could then specify --devel to install it.

Another option would be to move things like amber into their own tap(s) until crystal hits 1.0.

@matiasgarciaisaia
Copy link
Contributor Author

Just changed llvm@5 to llvm.

@DomT4
Copy link
Member

DomT4 commented Aug 17, 2018

@BrewTestBot test this please

@DomT4 DomT4 closed this in 3eb9fe9 Aug 17, 2018
@DomT4
Copy link
Member

DomT4 commented Aug 17, 2018

Merged in 3eb9fe9. Thank you for the contribution @matiasgarciaisaia, and thanks to everyone who helped out with review/discussion/etc.

@lock lock bot added the outdated PR was locked due to age label Sep 16, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 16, 2018
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.

6 participants