crystal: change all crystal programs to use buildCrystalPackage and update pkgs #83054

peterhoeg · 2020-03-21T05:26:46Z

Motivation for this change

See the last comments for more details.

~~The icr change is straight forward and only included here to showcase the change in how we build crystal packages.~~

~~The crystal builder will now use a Makefile if one is included by the project, shards if we have the required files in place and otherwise build the specified binaries (the old behaviour).~~

Couple of other things to note:

we add a few more things to nativeBuildInputs that are required by several crystal programs.
upstream recommends not building with --no-debug so rip that out
support copying in a lockfile if the program doesn't have one (lucky-cli I'm looking at you)

~~The way I've done the conditionals where we combine bash conditionals with nix conditionals is very ugly and I'm not proud of that at all but it works.~~

Cc @manveru as the person we have to thank for having the crystal build infrastructure in the first place.

Things done

manveru

Looks good to me :)

peterhoeg · 2020-04-12T12:29:57Z

Thanks for the feedback. I just made the build and install targets configurable, but otherwise this version is the same. I have nixpkgs-fmt running in a save hook, so that's the reason for all the noise.

peterhoeg · 2020-04-12T12:55:19Z

@GrahamcOfBorg build crystal2nix mint scry gitAndTools.thicket

manveru · 2020-04-17T16:17:43Z

Shards now depend on buildCrystalPackage to build, since they have introduced a shard as a dependency... so now we get infinite recursion with this.

peterhoeg · 2020-04-17T23:32:33Z

now we get infinite recursion with this.

I saw that last night as well. Upstream was discussing distributing shards with the dependency vendored to avoid having to depend on itself. Maybe we could just do that to avoid the problem until they figure out what they want. I'm open to ideas.

manveru · 2020-04-19T12:16:18Z

It'd be nice if we allowed an attrset instead of a path for the shardsFile. That way you could inject your own versions of shards more easily. Maybe also allowing JSON format for the shards file, so the generator can be simpler and faster...

peterhoeg · 2020-04-19T15:21:18Z

I'm just seeing your comment about the attrset now, so that has not been addressed.

I hijacked my own PR to clean up the various crystal bits we carry to have them all up to date and using buildCrystalPackage. Few things to note:

we now run tests unless disabled
instead of the fugly bash/nix combo that tries to find the right thing to build, we simply make it configurable similar to how it works for python using a format attr.
I have a vendored version of shards which I think we should stick under the nixos/nix-community org for the future.
some of them are pinned against older crystal versions. I did that directly with each program instead of all-packages.nix to avoid forgetting about it at the next update cycle. I don't use mint, so I haven't tried that. But it compiles and the tests pass.
I had initially undone your work to move shards to buildCrystalPackage but with the configurable format we avoid the cyclical dependency.

peterhoeg · 2020-04-22T03:21:46Z

If nobody has any objections (@manveru @kimburgess @filalex77) I'll be merging this soonish.

kimburgess · 2020-04-22T06:10:10Z

Changes to Ameba look good 👍

I've only had time for a shallow read so am not familiar with the current limitations, but working to move away from using that vendored shards repo seems like a neater long term. Possibly worth spinning up a separate issue for tracking / resolving that.

Mic92 · 2020-04-22T06:43:34Z

pkgs/development/tools/build-managers/shards/default.nix

-    repo   = "shards";
-    rev    = "v${version}";
-    sha256 = "1bjy3hcdqq8769bx73f3pwn26rnkj23dngyfbw4iv32bw23x1d49";
+    owner = "peterhoeg";


Are you the new maintainer of shards?

Ah. This is the vendored version. Maybe nix-community would be a better place for this, so we can add more contributors easier.

I actually think we can do without the vendored version. Let me take a look.

And we can. Plus we now get man pages as well.

peterhoeg · 2020-04-22T07:37:47Z

@GrahamcOfBorg build ameba crystal2nix icr mint scry shards gitAndTools.thicket

peterhoeg · 2020-04-22T07:50:06Z

@GrahamcOfBorg build ameba crystal2nix icr mint scry shards gitAndTools.thicket

manveru · 2020-04-22T09:18:56Z

@peterhoeg seems like it still fails on missing installManPage.
If all builds pass, I'm all for merging this.

peterhoeg · 2020-04-22T09:32:05Z

I was working against unstable but when rebasing against master, I got the error too. Ahhh, well. It's fixed now.

peterhoeg · 2020-04-22T12:31:24Z

Thanks for your feedback guys, I'm merging this.

Artturin · 2022-10-17T13:03:09Z

pkgs/development/tools/icr/default.nix

-  checkTarget = "test";
+  postFixup = ''
+    wrapProgram $out/bin/icr \
+      --prefix PATH : ${lib.makeBinPath [ crystal shards makeWrapper which ]}


the makeWrapper here looks like a mistake so i'll remove it

peterhoeg requested a review from manveru March 21, 2020 05:26

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Mar 21, 2020

manveru approved these changes Apr 12, 2020

View reviewed changes

peterhoeg force-pushed the u/icr branch from 8afa9b8 to fef1f7b Compare April 12, 2020 12:27

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 12, 2020

peterhoeg force-pushed the u/icr branch from 1c02a6c to 844b157 Compare April 12, 2020 14:48

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 12, 2020

ofborg bot requested a review from manveru April 12, 2020 14:55

peterhoeg force-pushed the u/icr branch from 844b157 to 8232f3a Compare April 19, 2020 07:35

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 19, 2020

peterhoeg force-pushed the u/icr branch 2 times, most recently from 9605c62 to 635a67e Compare April 19, 2020 15:13

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 19, 2020

ofborg bot requested review from Br1ght0ne and kimburgess April 19, 2020 15:21

peterhoeg changed the title ~~icr: 0.6.0 -> 0.8.0 and use shards to build crystal packages~~ crystal: change all crystal programs to use buildCrystalPackage Apr 19, 2020

peterhoeg changed the title ~~crystal: change all crystal programs to use buildCrystalPackage~~ crystal: change all crystal programs to use buildCrystalPackage and update pkgs Apr 19, 2020

peterhoeg force-pushed the u/icr branch from 635a67e to cafad37 Compare April 19, 2020 15:27

This was referenced Apr 19, 2020

crystal.buildCrystalPackage: add checkPhase #74919

Closed

crystal: use llvm 10 exclusively #85445

Merged

Mic92 reviewed Apr 22, 2020

View reviewed changes

peterhoeg force-pushed the u/icr branch 2 times, most recently from 9c9c565 to 1047042 Compare April 22, 2020 07:36

peterhoeg added 9 commits April 22, 2020 17:30

crystal: build using Makefile or shards if available

912eb6b

shards: 0.9.0 -> 0.10.0

760d56f

icr: 0.6.0 -> 0.8.0

1e3bae6

mint: 0.7.1 -> 0.9.0

fb8902b

scry: build against crystal 0.31

4805be6

ameba: 0.12.0 -> 0.12.1

61660a7

lucky-cli: init at 0.20

26a69b3

crystal2nix: specify build type

8197960

thicket: specify the build type and pin to crystal 0.33

0bfd455

peterhoeg force-pushed the u/icr branch from 1047042 to 0bfd455 Compare April 22, 2020 09:30

peterhoeg merged commit f690b34 into NixOS:master Apr 22, 2020

peterhoeg deleted the u/icr branch April 22, 2020 12:33

Artturin reviewed Oct 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crystal: change all crystal programs to use buildCrystalPackage and update pkgs #83054

crystal: change all crystal programs to use buildCrystalPackage and update pkgs #83054

peterhoeg commented Mar 21, 2020 •

edited

Loading

manveru left a comment

peterhoeg commented Apr 12, 2020

peterhoeg commented Apr 12, 2020

manveru commented Apr 17, 2020

peterhoeg commented Apr 17, 2020

manveru commented Apr 19, 2020

peterhoeg commented Apr 19, 2020

peterhoeg commented Apr 22, 2020

kimburgess commented Apr 22, 2020

Mic92 Apr 22, 2020

Mic92 Apr 22, 2020

peterhoeg Apr 22, 2020

peterhoeg Apr 22, 2020

peterhoeg commented Apr 22, 2020

peterhoeg commented Apr 22, 2020

manveru commented Apr 22, 2020

peterhoeg commented Apr 22, 2020

peterhoeg commented Apr 22, 2020

Artturin Oct 17, 2022

crystal: change all crystal programs to use buildCrystalPackage and update pkgs #83054

crystal: change all crystal programs to use buildCrystalPackage and update pkgs #83054

Conversation

peterhoeg commented Mar 21, 2020 • edited Loading

Motivation for this change

Things done

manveru left a comment

Choose a reason for hiding this comment

peterhoeg commented Apr 12, 2020

peterhoeg commented Apr 12, 2020

manveru commented Apr 17, 2020

peterhoeg commented Apr 17, 2020

manveru commented Apr 19, 2020

peterhoeg commented Apr 19, 2020

peterhoeg commented Apr 22, 2020

kimburgess commented Apr 22, 2020

Mic92 Apr 22, 2020

Choose a reason for hiding this comment

Mic92 Apr 22, 2020

Choose a reason for hiding this comment

peterhoeg Apr 22, 2020

Choose a reason for hiding this comment

peterhoeg Apr 22, 2020

Choose a reason for hiding this comment

peterhoeg commented Apr 22, 2020

peterhoeg commented Apr 22, 2020

manveru commented Apr 22, 2020

peterhoeg commented Apr 22, 2020

peterhoeg commented Apr 22, 2020

Artturin Oct 17, 2022

Choose a reason for hiding this comment

peterhoeg commented Mar 21, 2020 •

edited

Loading