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 package builder #67510

Merged
merged 6 commits into from Aug 29, 2019

Conversation

@Infinisil
Copy link
Member

commented Aug 26, 2019

Motivation for this change

I'll be trying to package invidious soon, which uses crystal, so I thought why not first upgrade the crystal infrastructure in nixpkgs a bit.

I put the buildCrystalPackage function inside the compiler attributes such that you can select a version with e.g. pkgs.crystal_0_29.buildCrystalPackage, which is very simple.

I recommend viewing each commit separately

Ping @manveru @david50407 @peterhoeg

Things done
  • Built and tested all of the two crystal packages (the new crystal2nix and mint)
  • Ensured that relevant documentation is up to date. I don't really feel like writing docs right now
Infinisil added 3 commits Aug 26, 2019

defaultOptions = [ "--release" "--progress" "--no-debug" "--verbose" ];

in stdenv.mkDerivation (mkDerivationArgs // {

This comment has been minimized.

Copy link
@manveru

manveru Aug 26, 2019

Contributor

Wouldn't it be nicer to allow overriding of all attributes by inverting the merge order?

This comment has been minimized.

Copy link
@Infinisil

Infinisil Aug 26, 2019

Author Member

@manveru For all attributes I'm setting in the derivation, I'm also propagating the ones from args (if any). This order is needed because otherwise e.g. the users buildInputs would override our changed buildInputs which contains crystal.

runHook postInstall
'';

meta = args.meta or {} // {

This comment has been minimized.

Copy link
@manveru

manveru Aug 26, 2019

Contributor

Not sure why the {} // is needed here.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Aug 26, 2019

Author Member

It's parsed as (args.meta or {}) // { ... }, so this prevents it from failing when args doesn't have meta

runHook postConfigure
'';

buildInputs = args.buildInputs or [] ++ [ crystal ];

This comment has been minimized.

Copy link
@manveru

manveru Aug 26, 2019

Contributor

Is the [] ++ needed here?

This comment has been minimized.

Copy link
@Infinisil

Infinisil Aug 26, 2019

Author Member

Yeah same reason, in case args doesn't have buildInputs set

This comment has been minimized.

Copy link
@manveru

manveru Aug 26, 2019

Contributor

Sorry, I was reading the precedence wrong... really would prefer parenthesis in this case :)

@peterhoeg

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

We definitely need this! Any reason for building using crystal instead of shards?

@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@peterhoeg I have zero experience with both crystal and shards, I just followed what the mint derivation was doing. Maybe crystal is preferable here because it also allows the compilation of the single crystal2nix.cr file, whereas shards I'd think would need a shard.yaml file.

shards seems to automatically know what file to build though, so maybe that would be nice to support too (in the future?).

Infinisil added 2 commits Aug 26, 2019
@Infinisil Infinisil force-pushed the Infinisil:crystal-infra branch from 05438bf to 1d07491 Aug 26, 2019
@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

I pushed a minor mint change for removing all buildInputs except openssl, because the others didn't end up being needed

I also added a commit for adding a Crystal section to the nixpkgs docs

Copy link
Contributor

left a comment

nix-review passes on NixOS
diff LGTM (But im also very unfamiliar with crystal, and it should probably be tested by building a crystal package)
has documentation 👍
executables still seem to work

[4 built, 33 copied (1293.6 MiB), 223.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/67510
2 package were build:
crystal2nix mint
@peterhoeg

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

whereas shards I'd think would need a shard.yaml file

When you do crystal init app foo, it will create a shard.yml file with (amongst other things), this:

targets:
  foo:
    main: src/foo.cr

Which is enough for you to build with shards build.

Except for crystal2nix.cr, I don't think anybody invokes the crystal compiler directly.

@Infinisil Infinisil referenced this pull request Aug 29, 2019
2 of 2 tasks complete
@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

@peterhoeg Since we need this functionality anyways for crystal2nix, I'd say let's just leave it at this for now. Adding a way of using shards to build packages is probably not worth it right now because there's almost no crystal packages, can always be added later though.

This makes `crystal play` work
@Infinisil

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Just added a commit to switch crystal to openssl_1_0_2 which makes crystal play work.

@ofborg ofborg bot requested review from manveru and peterhoeg Aug 29, 2019
@peterhoeg

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

I'd say let's just leave it at this for now.

Fair enough. It's definite improvement on the existing situation. lgtm when the darwin test passes.

@Infinisil Infinisil merged commit bfb06ec into NixOS:master Aug 29, 2019
15 of 16 checks passed
15 of 16 checks passed
crystal, crystal2nix, mint on aarch64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
crystal, crystal2nix, mint on x86_64-darwin Success
Details
crystal, crystal2nix, mint on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Infinisil Infinisil deleted the Infinisil:crystal-infra branch Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.