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

Use context instead of nested its to fix breaking changes in Crystal .27.1 #202

Merged
merged 2 commits into from
Feb 5, 2019

Conversation

Jens0512
Copy link
Member

@Jens0512 Jens0512 commented Feb 2, 2019

As of Crystal .27.1 nested it's are no longer allowed (crystal-lang/crystal#7297), and it raises at runtime. The breaking change affects 6 of our it's, hence the specs are broken.

This PR simply changes the parent it's to describe's which fixes the problem.

@Jens0512
Copy link
Member Author

Jens0512 commented Feb 2, 2019

The CI is broken: No command 'crystal' found. 👀

Seems we're not the only ones with failing crystal on travis: https://travis-ci.community/t/apt-get-install-crystal-0-27-1-failed/2093

@faultyserver faultyserver added the spec An issue with relating to the specs around the Myst codebase (native or in-language). label Feb 2, 2019
@faultyserver
Copy link
Member

Seems good to me. Will merge once they get the CI image updated. I suppose also updating shard.yml and .tool-versions to 0.27.1 would be good to do here as well.

That said I still don't understand why they've been so adamant to not allow users to specify the version of Crystal they're using. There's only latest and nightly, and even nightly has almost never worked.

@Jens0512
Copy link
Member Author

Jens0512 commented Feb 3, 2019

Fixed, according to bcardiff of the Crystal core team.

@faultyserver
Copy link
Member

Looks like it's still failing because of the changes to integer overflow handling.

@Jens0512
Copy link
Member Author

Jens0512 commented Feb 3, 2019

That's weird, the overflow behavior should be disabled unless -D preview_overflow is passed to the compiler.

From the PR merging the overflows:

In order to avoid breaking changes and to allow a preview of this feature, the overflow behavior is only enabled if compiled with -D preview_overflow.

@faultyserver
Copy link
Member

Very. I haven't pulled and run it myself to see what exactly is causing it, it might just be a stack recursion overflow or something else.

In any case, I would imagine it's reproducible.

@Jens0512
Copy link
Member Author

Jens0512 commented Feb 3, 2019

It works perfectly well for me: asciineema recording (Finally a reason to try asciinema!).

The OverflowError on the CI is raised during the compilation of spec/all_spec.cr, which means the error is with the compiler. But I fail to make sense of how it fails on the CI, but not on my machine. I've got a slightly different version: Crystal 0.27.1 (2019-02-02), while the CI says it has Crystal 0.27.1 [64137d045] (2019-01-30), but I dunno if that has much to say...

@Jens0512
Copy link
Member Author

Jens0512 commented Feb 4, 2019

The CI 64137d045 crystal version ref is exactly the 0.27.1 release. The only thing that I can imagine makes sense is that the CI has some kind of limitation on its resources, which makes the build fail. However I fail to see how it would raise an OverflowError, I mean, it's an integer overflow, right? The int max and min values are the same on the CI and my computer are the same, as both are x64 systems, AFAIK crystal doesn't even support 32-bit architechtures. Alternatively, might this be a bug in the compiler?

@Jens0512
Copy link
Member Author

Jens0512 commented Feb 4, 2019

Yep, it's the compiler.

.tool-versions Outdated Show resolved Hide resolved
spec/interpreter/nodes/op_assign_spec.cr Outdated Show resolved Hide resolved
@Jens0512
Copy link
Member Author

Jens0512 commented Feb 4, 2019

There, now we just wait for .27.2. Shouldn't be long.

@Jens0512 Jens0512 changed the title Use describe in place of nested its to fix breaking changes in Crystal 0.27.1 Use context instead of nested its to fix breaking changes in Crystal .27.1 Feb 4, 2019
@faultyserver faultyserver merged commit 0057a7d into myst-lang:master Feb 5, 2019
@Jens0512 Jens0512 deleted the crystal-0.27.1 branch February 5, 2019 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec An issue with relating to the specs around the Myst codebase (native or in-language).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants