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-lang 0.25.0 #28930

Closed

Conversation

felixbuenemann
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>)?

  • Bump crystal-lang to 0.25.0
  • Bump shards resource to 0.8.0
  • Bump boot resource to 0.24.2

@ilovezfs
Copy link
Contributor

https://github.com/crystal-lang/crystal/releases/latest is still 0.24.2 but thanks for the PR nonetheless @felixbuenemann

@ilovezfs ilovezfs closed this Jun 12, 2018
@ilovezfs
Copy link
Contributor

CC @matiasgarciaisaia

@matiasgarciaisaia
Copy link
Contributor

We're about to release, but not yet. Will let you know when it's done.

Thanks, @ilovezfs 👍

@ilovezfs
Copy link
Contributor

you're welcome!

@felixbuenemann
Copy link
Contributor Author

@matiasgarciaisaia Just mention me here if it's ready and I'll resubmit the PR.

@matiasgarciaisaia
Copy link
Contributor

@ilovezfs Linux repos have just been published with 0.25.0.

We'll make the official announcement in the next hours - if you merge here, we'll announce every platform at the same time 👌

Thanks @felixbuenemann for the PR :)

@ilovezfs ilovezfs reopened this Jun 15, 2018
@felixbuenemann
Copy link
Contributor Author

felixbuenemann commented Jun 15, 2018

@matiasgarciaisaia Are the 0.24.2 boot binaries supposed to be compatible with El Capitan?

Jenkins shows the following build failure:

05:03:54 ==> bin/crystal build -D without_openssl -D without_zlib -o .build/crystal src/compiler/crystal.cr --release --no-debug
05:03:54 dyld: _dyld_bind_fully_image_containing_address() error
05:03:54 dyld: Symbol not found: _clock_gettime
05:03:54   Referenced from: /private/tmp/crystal-lang-20180615-33906-1so781b/crystal-0.25.0/boot/embedded/bin/crystal (which was built for Mac OS X 10.12)
05:03:54   Expected in: /usr/lib/libSystem.B.dylib
05:03:54  in /private/tmp/crystal-lang-20180615-33906-1so781b/crystal-0.25.0/boot/embedded/bin/crystal
05:03:54 /private/tmp/crystal-lang-20180615-33906-1so781b/crystal-0.25.0/boot/bin/crystal: line 102: 33991 Trace/BPT trap: 5       "$INSTALL_DIR/embedded/bin/crystal" "$@"

The failures on Sierra and High Sierra are related to failures in rebuilding the amber formula.

I tried that locally using brew install -vd --build-from-source amber and it seems the ameba shard needs to be updated to work with the new version:

Installing ameba (0.5.1)
Postinstall make bin
Failed make bin:
/usr/local/opt/crystal-lang/bin/shards build --no-debug
Dependencies are satisfied
Building: ameba
Error target ameba failed to compile:
Error in src/cli.cr:3: instantiating 'Ameba::Cli:Module#run(Array(String))'

Ameba::Cli.run(ARGV)
           ^~~

in src/ameba/cli/cmd.cr:10: instantiating 'Ameba::Config:Class#load(String)'

    config = Config.load opts.config
                    ^~~~

in src/ameba/config.cr:47: instantiating 'Ameba::Config:Class#new(YAML::Any)'

    Config.new YAML.parse(content)
           ^~~

in src/ameba/config.cr:32: instance variable '@rules' of Ameba::Config must be Array(Ameba::Rule::Base), not Array(Ameba::Rule::ComparisonToBoolean | Ameba::Rule::ConstantNames | Ameba::Rule::DebuggerStatement | Ameba::Rule::EmptyEnsure | Ameba::Rule::EmptyExpression | Ameba::Rule::HashDuplicatedKey | Ameba::Rule::LargeNumbers | Ameba::Rule::LineLength | Ameba::Rule::LiteralInCondition | Ameba::Rule::LiteralInInterpolation | Ameba::Rule::MethodNames | Ameba::Rule::NegatedConditionsInUnless | Ameba::Rule::PercentArrays | Ameba::Rule::PredicateName | Ameba::Rule::RandZero | Ameba::Rule::RedundantBegin | Ameba::Rule::ShadowedException | Ameba::Rule::Syntax | Ameba::Rule::TrailingBlankLines | Ameba::Rule::TrailingWhitespace | Ameba::Rule::TypeNames | Ameba::Rule::UnlessElse | Ameba::Rule::UnneededDisableDirective | Ameba::Rule::UselessConditionInWhen | Ameba::Rule::VariableNames | Ameba::Rule::WhileTrue)

    @rules = Rule.rules.map &.new(config)
    ^~~~~~


make: *** [build] Error 1
/usr/local/Homebrew/Library/Homebrew/debrew.rb:11:in `raise'

Looks like these issues are already tracked upstream:

@ilovezfs
Copy link
Contributor

@matiasgarciaisaia Also, it looks like the build fails with LLVM 6, which is disappointing as we seem to always have to use an old version of llvm here. Is this being addressed upstream?

@bcardiff
Copy link
Contributor

@ilovezfs
Crystal should build fine in llvm 6.0.1 (but not 6.0.0 ref:crystal-lang/crystal#5556 (comment)). But using llvm 5 fine (yet I understand the disappointment).

@felixbuenemann
Regarding el capitan issue, 0.24.2 is the first binary to be built in a new ci that runs on CircleCI 2.0 and seems that there is no container for el capitan there. But other than building them in 10.12 Sierra there was no other action regarding compatibility with el capitan.

I'm not sure what should be the next steps.

@felixbuenemann
Copy link
Contributor Author

Regarding el capitan issue, 0.24.2 …

@ilovezfs already solved that issue by using the 0.24.1 boot binaries when building on El Capitan.

I'm not sure what should be the next steps.

Normally, if you bump a formula and that causes other formulas that depend on it to fail, you would update those formulae to newer compatible versions in the same PR.

Currently there is no amber versions that's compatible with 0.25.0 so we'll have to wait until that lands.

@ilovezfs
Copy link
Contributor

Is it possible to backport a patch for amber?

@felixbuenemann
Copy link
Contributor Author

Is it possible to backport a patch for amber?

I tried, but there are a lot of dependencies that are also incompatible.

I came as far as patching amber, bumping the ameba shard to master, bumping teeplate to 0.6.0 and switching back from the fork to the original repo and then ended up with build failures in the pg shard, which doesn't appear to have any updates for 0.25.0 yet int the master branch.

At that point I decided that making the existing version compatible wasn't trivial and aborted the patch.

@faustinoaq
Copy link

Hi all, amber maintainer here :)

Currently there is no amber versions that's compatible with 0.25.0 so we'll have to wait until that lands.

We're expecting a new release very very soon

I think we need to include this patch amberframework/amber#826 before a new amberframework version /cc @eliasjpr @elorest @drujensen @robacarp

@asterite
Copy link
Contributor

asterite commented Jun 15, 2018

I don't think it's OK for tests to fail because amber depends on Crystal.

Does that mean that whenever we want to release Crystal in homebrew we'll have to wait for the Amber team to update their homebrew formula, before Crystal is released in homebrew? Kind of a recursive problem. Plus, it makes Crystal come later to everyone, even those that don't use Amber.

And what if later more tests/checks for other tools that use Crystal are added, the process will become slower and slower.

Please, remove the check against amber. Amber can be updated later after Crystal is released.

@ilovezfs
Copy link
Contributor

@asterite That is the price of your making API breaking changes to crystal. test-bot always tests all reverse dependencies for run-time breakage, which is how we ensure that users can run brew upgrade without breaking what they already have installed. That test-bot behavior cannot be selectively disabled, but we also have the choice to ship this PR while it's failing CI.

We have some choices:

  1. Deem it temporarily acceptable that anyone who has amber installed and runs brew upgrade will have broken their installation.
  2. Carry multiple versions of crystal in Homebrew – in this case a new crystal@0.24 formula. Typically we only add such formulae if upstream maintains a separate release branch for 0.24.x, which I don't believe is the case for crystal. Sometimes we will add such a formula as a short term measure even though there is no such separately maintained release branch, and remove it as soon as it's no longer needed.
  3. Block the crystal upgrade in Homebrew until either there is a version of amber that works with it or a patch can be backported to the current version of amber.

@RX14
Copy link
Contributor

RX14 commented Jun 15, 2018

@asterite this is just how the traditional package repository method works: keeping software up to date is secondary to reducing breakage for users. Not everyone needs the latest version of crystal all the time. Option 3 is fine.

@asterite
Copy link
Contributor

Yeah, I guess option 3 is fine...

@ilovezfs
Copy link
Contributor

Given that amber upstream has indicated a new release is imminent, I'm going to opt for option 1.

@ilovezfs ilovezfs closed this in 1ac6740 Jun 15, 2018
@ilovezfs
Copy link
Contributor

Thanks for the PR @felixbuenemann and the upgrade @matiasgarciaisaia @asterite @RX14

@Willamin
Copy link

That is the price of your making API breaking changes to crystal.

In Crystal's documentation it is very clearly expressed that due to it being still very much in development and not at 1.0 yet, API breaking changes are expected. If someone is using Crystal in production in such a way that updating Crystal could break their usage, they should be pinning the version of Crystal that works.

I'm disappointed in the decision to hold on this PR until other packages like amber are updated to work with the latest Crystal version.

😞

@ilovezfs
Copy link
Contributor

@Willamin I just shipped this PR, so …

@asterite
Copy link
Contributor

@ilovezfs Thank you. Your handling of PRs across this repository is exceptional!

@ilovezfs
Copy link
Contributor

@asterite you're welcome and thank you for the kind words ❤️

@felixbuenemann
Copy link
Contributor Author

Btw. I think it would also be an option to not accept formulae that depend on crystal-lang into homebrew-core until crystal reaches 1.0 and it's api is deemed stable.

@felixbuenemann felixbuenemann deleted the crystal-lang-0.25.0 branch June 15, 2018 14:39
@ilovezfs
Copy link
Contributor

Amber is now fixed! #29299

@matiasgarciaisaia matiasgarciaisaia mentioned this pull request Jun 28, 2018
4 tasks
@asterite
Copy link
Contributor

@felixbuenemann Why did you add that check against El Capitan? Crystal for mac doesn't use clock_gettime, nor it defines it at all. So why did you end up doing that? What failed?

@felixbuenemann
Copy link
Contributor Author

@asterite It was added by ilovezfs in dd8f816 because the 0.24.2 binaries don't work on El Capitan.

See #28930 (comment) from bcardiff for details.

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

8 participants