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.31.1 -> 0.32.1 #75545

Merged
merged 3 commits into from
Jan 30, 2020
Merged

crystal: 0.31.1 -> 0.32.1 #75545

merged 3 commits into from
Jan 30, 2020

Conversation

kimburgess
Copy link
Member

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @manveru @david50407 @peterhoeg

Note: it may be worth switching building inputs across to use llvm_8 so that it's matched to the official builds.

Copy link
Contributor

@manveru manveru left a comment

Choose a reason for hiding this comment

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

Could you also add this version to the top-level?

@manveru manveru mentioned this pull request Dec 12, 2019
10 tasks
@peterhoeg
Copy link
Member

Note: it may be worth switching building inputs across to use llvm_8 so that it's matched to the official builds.

Do you know from which version onwards?

@kimburgess
Copy link
Member Author

Do you know from which version onwards?

Looks like it bumped as part of 0.32.0. See crystal-lang/crystal@47b68e9, which points to crystal-lang/distribution-scripts@6e8201b

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Dec 13, 2019
@ofborg ofborg bot requested a review from manveru December 13, 2019 00:38
@Br1ght0ne
Copy link
Member

Br1ght0ne commented Dec 13, 2019

@GrahamcOfBorg build crystal
Just checking if Darwin build passes.

@manveru
Copy link
Contributor

manveru commented Dec 16, 2019

@filalex77 any update on this?

@peterhoeg
Copy link
Member

@kimburgess, upstream changed from llvm 4 to 8, while we are currently using llvm 7 so looks like crystal isn't too picky about the llvm version. I suggest we don't do anything special and just use whatever is the default llvm in nixpkgs.

@manveru
Copy link
Contributor

manveru commented Jan 18, 2020

There was a 0.32.1 release in the meanwhile. @kimburgess

@kimburgess kimburgess changed the title crystal: 0.31.1 -> 0.32.0 crystal: 0.31.1 -> 0.32.1 Jan 19, 2020
@ofborg ofborg bot requested a review from manveru January 19, 2020 14:28
@manveru
Copy link
Contributor

manveru commented Jan 19, 2020

Seems like there's a linux build failure still, so the specs probably shouldn't be enabled after all.

@kimburgess
Copy link
Member Author

Ah yes, sorry about that - I was still playing around with work on those specs.

I've now added patches to either:

This lets the check phase run for some added sanity / confirmation that the build is stable.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 20, 2020
@peterhoeg
Copy link
Member

You don't need to package the binary derivations - we should be self-hosted using current - 1.

@kimburgess
Copy link
Member Author

@peterhoeg how far back do you want to go with that? They were added to match the build pattern used by 0.30.x and earlier. The potential danger in chaining builds is a machine without access to a cache for n - 1 will be forced to build every compiler version up until the release of interest. Definitely makes sense for when there's not such a tight dependency between versions, but not sure if that's now?

P.S. I'll rebase and neaten up this scattering of commits for you prior to merge too.

@RX14
Copy link

RX14 commented Jan 23, 2020

Definitely makes sense for when there's not such a tight dependency between versions, but not sure if that's now?

The last release will build the next release is the guarantee.

Bootstrapping crystal from the original ruby version is possible but takes hours (and is only possible on x86). Depends if your aim is to have maximum trust for everything, or whether your aim is to support building without a binary cache or what the tradeoff you want to make is.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-20-03-feature-freeze/5655/7

@kimburgess
Copy link
Member Author

@manveru re the discourse mention: still here.

I’ve been awaiting an answer from @peterhoeg on the binary dependency. Sorry if that was unclear.

As a co-maintainer do either of you have a preference?

@manveru
Copy link
Contributor

manveru commented Jan 29, 2020

I don't mind having a binary dependency, it really cuts down the build times when changing the generic derivation, which needs to rebuild all the versions before it as well.

As an experiment I also once tried building all versions all the way back to the Ruby one, but turns out we don't have the required ancient LLVM version in any revision of nixpkgs, so I gave up on that.

While from a theoretical and security perspective, having a full bootstrap would probably be beneficial.
It's just not very pragmatic, given that a single build of Crystal takes almost 45 minutes on my machine and we'll most likely have to change the derivation in future.

Another option would be to split up the generic derivation into "frozen" ones, and keep one for each version that won't be touched in future. But even then it will be a major drag for mass rebuilds if any of the dependencies change.

What I would like instead is actually reducing the number of versions we keep. Crystal is still a pretty fast moving target, and if one really needs older versions they can use a specific checkout of nixpkgs. I don't think it makes sense to have anything older than 1 year in the available versions, nobody tests them to make sure they are still working, not to mention we have checks disabled for them as well, so even if they still build, they might be broken at any time.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 30, 2020
@ofborg ofborg bot requested a review from manveru January 30, 2020 04:22
@kimburgess
Copy link
Member Author

Cool. Deprecating / removing old versions is probably best handled outside of this PR. Definitely agree that limiting available builds makes sense in order to keep noise to a minimum in the top level packages.

I've tidied things up for what's needed here for now.

@peterhoeg
Copy link
Member

Thanks for all the hard work on the tests @kimburgess. Let's reassess the situation with binary builders when crystal hits 1.0. I'm with both you and @manveru on this.

@peterhoeg peterhoeg merged commit 306e1f9 into NixOS:master Jan 30, 2020
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Jan 30, 2020
crystal: 0.31.1 -> 0.32.1
(cherry picked from commit 306e1f9)
anna328p pushed a commit to anna328p/nixpkgs that referenced this pull request Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants