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: 1.11 -> 1.14 #303173

Merged
merged 10 commits into from
Oct 27, 2024
Merged

crystal: 1.11 -> 1.14 #303173

merged 10 commits into from
Oct 27, 2024

Conversation

will
Copy link
Contributor

@will will commented Apr 10, 2024

Current Description of changes

Release notes: https://crystal-lang.org/2024/10/09/1.14.0-released/

Old Description of changes

Release notes: https://crystal-lang.org/2024/04/09/1.12.0-released

From the notes:

Our own LLVM extension library libllvm_ext is no longer required when linking LLVM >= 18 because all APIs are now directly exposed in libllvm directly. The Makefile automatically skips building libllvm_ext.o on LLVM 18 and above.

So I've updated the llvm package for this version to llvm 18. From talking with people on the Crystal core team, there is very small chance that the 16-bit alignment for 128-bit integers in llvm18 can cause problems on x86 systems (not aarch64 since they already were using 16-bit alignment), and if so we should go down to llvm 17.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage

Result of nixpkgs-review run on aarch64-darwin 1

1 package marked as broken and skipped:
  • rtfm
3 packages failed to build:
  • collision
  • crystalline
  • invidious
18 packages built:
  • ameba
  • amqpcat
  • blahaj
  • crystal
  • crystal.bin
  • crystal.lib
  • crystal2nix
  • gi-crystal
  • icr
  • kakoune-cr
  • lucky-cli
  • mint
  • oq
  • scry
  • shards (shards_0_17)
  • thicket
  • tijolo
  • tmuxPlugins.fingers
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@cole-h
Copy link
Member

cole-h commented Apr 10, 2024

You'll want to rebase on the latest master -- there was an issue that broke ofborg's ability to evaluate Nixpkgs that has since been resolved. Sorry for the inconvenience!

@will
Copy link
Contributor Author

will commented Apr 11, 2024

I've also now fixed crystalline by making sure to pick the same version of llvm. And I've patched a type error in invidious that I would assume they'll get to eventually in a new version and nixpkgs can drop that small patch when it updates to a newer version of invidious.

The collision package, however, I can't get to work. There is an error generated while running its Makefile that is from gi-crystal. No combination of pinning back to previous versions of crystal for either collision and/or gi-crystal seems to fix it, so I've marked it as broken for now.

Result of nixpkgs-review run on aarch64-darwin 1

2 packages marked as broken and skipped:
  • collision
  • rtfm
23 packages built:
  • ameba
  • amqpcat
  • blahaj
  • crystal (crystal_1_12)
  • crystal.bin (crystal_1_12.bin)
  • crystal.lib (crystal_1_12.lib)
  • crystal2nix
  • crystal_1_11
  • crystal_1_11.bin
  • crystal_1_11.lib
  • crystalline
  • gi-crystal
  • icr
  • invidious
  • kakoune-cr
  • lucky-cli
  • mint
  • oq
  • scry
  • shards (shards_0_17)
  • thicket
  • tijolo
  • tmuxPlugins.fingers

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Apr 11, 2024
@ofborg ofborg bot requested review from sbruder and sund3RRR April 11, 2024 08:28
@peterhoeg
Copy link
Member

We don't share homebrew's problems, but I just want to bring attention to crystal-lang/crystal#14318 to see if anyone has any issues that this might solve.

@will
Copy link
Contributor Author

will commented Apr 11, 2024

@peterhoeg I saw that patch too, and while I'm not 100% sure, I think think the long stdenv wrapper scripts around the linker and whatnot take care of the problems that patch is trying to address

@will will force-pushed the update-crystal-1.12 branch from cafe30b to cafef58 Compare April 19, 2024 11:35
@sund3RRR
Copy link
Contributor

I can't compile anything on EndeavourOS regardless of crystal version

nix-build -A tijolo
this derivation will be built:
  /nix/store/pwlcp6r8x2lwg6d0xq0aficw319nj3bw-tijolo-0.7.4.drv
building '/nix/store/pwlcp6r8x2lwg6d0xq0aficw319nj3bw-tijolo-0.7.4.drv'...
Running phase: unpackPhase
unpacking source archive /nix/store/qiv3kriqmw0i5kpqrgaqsb1nbrr6a1bx-source
source root is source
Running phase: patchPhase
Running phase: updateAutotoolsGnuConfigScriptsPhase
Running phase: configurePhase
Running phase: buildPhase
shards build --release -s  -Dpreview_mt --ignore-crystal-version
Dependencies are satisfied
Building: tijolo
Parse:                             00:00:00.000122163 (   1.07MB)
Error target tijolo failed to compile:
impure path `/homeless-shelter/.cache/crystal/build-source-lib-version_from_shard-src-extract_version.cr/macro_run' used in link
collect2: error: ld returned 1 exit status
Error: execution of command failed with exit status 1: gcc "${@}" -o /homeless-shelter/.cache/crystal/build-source-lib-version_from_shard-src-extract_version.cr/macro_run  -rdynamic -L/nix/store/vyz5cpkg7rli3236cy30dald1yj9mdch-boehm-gc-8.2.4/lib -L/nix/store/5sqdrc4jpr4vjiiqycyw8q4v3zchpdka-pcre2-10.43/lib -L/nix/store/jlqg0qc06nas4qcpqn8w9nnkgsvrxjxp-libevent-2.1.12/lib -L/nix/store/pqjjikxz0xb7g79gj1h75qbgxg2xkbkz-libyaml-0.2.5/lib -L/nix/store/zph9xw0drmq3rl2ik5slg0n2frw9lw5m-zlib-1.3.1/lib -L/nix/store/ii7r1shwrbl9rcv6nn6mfvih5qcb742p-libxml2-2.12.6/lib -L/nix/store/p25ghy7y53lyc834xnw5mrhfq096wa4x-openssl-3.0.13/lib -L/nix/store/pqjjikxz0xb7g79gj1h75qbgxg2xkbkz-libyaml-0.2.5/lib -lyaml -lpcre2-8 -L/nix/store/vyz5cpkg7rli3236cy30dald1yj9mdch-boehm-gc-8.2.4/lib -lgc -lpthread -ldl -lpthread -L/nix/store/jlqg0qc06nas4qcpqn8w9nnkgsvrxjxp-libevent-2.1.12/lib -levent_pthreads -levent -L/nix/store/jlqg0qc06nas4qcpqn8w9nnkgsvrxjxp-libevent-2.1.12/lib -levent -lrt -lpthread -ldl

make: *** [Makefile:5: all] Error 1
error: builder for '/nix/store/pwlcp6r8x2lwg6d0xq0aficw319nj3bw-tijolo-0.7.4.drv' failed with exit code 2

Probably it is a non-nixos issue, but I think this shouldn't happen.

@ofborg ofborg bot requested a review from peterhoeg April 19, 2024 12:59
@sund3RRR sund3RRR mentioned this pull request Apr 20, 2024
13 tasks
will added 9 commits October 11, 2024 14:57
While updating crystal for 1.14 this project breaks with

    In src/code_language.cr:46:18

     46 | mime = Gio.content_type_guess(file.to_s, contents[0, contents_size])
                     ^-----------------
    Error: wrong number of arguments for 'Gio.content_type_guess' (given 2, expected 4)

    Overloads are:
     - Gio.content_type_guess(filename : ::String, data : Pointer(UInt8), data_size : UInt64, result_uncertain : Pointer(Bool))
@will will force-pushed the update-crystal-1.12 branch from cafef58 to cafef48 Compare October 11, 2024 14:01
@will will changed the title crystal: 1.11 -> 1.12 crystal: 1.11 -> 1.14 Oct 11, 2024
@will
Copy link
Contributor Author

will commented Oct 11, 2024

Updated this pr to change to crystal 1.14 now

2 packages marked as broken and skipped:
rtfm tijolo

19 packages built:
ameba amqpcat blahaj crystal crystal.bin crystal.lib crystal2nix crystalline gi-crystal icr invidious kakoune-cr lucky-cli mint oq scry shards thicket tmuxPlugins.fingers

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4669

@will
Copy link
Contributor Author

will commented Oct 15, 2024

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/ysk-guide-on-getting-your-pr-merged/54513/2

@donovanglover
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 303173


x86_64-linux

⏩ 1 package marked as broken and skipped:
  • tijolo
❌ 21 packages failed to build:
  • ameba
  • amqpcat
  • blahaj
  • collision
  • crystal (crystal_1_14)
  • crystal.bin (crystal_1_14.bin)
  • crystal.lib (crystal_1_14.lib)
  • crystal2nix
  • crystalline
  • gi-crystal
  • icr
  • invidious
  • kakoune-cr
  • lucky-cli
  • mint
  • oq
  • rtfm
  • scry
  • shards (shards_0_17)
  • thicket
  • tmuxPlugins.fingers
✅ 6 packages built:
  • crystal_1_11
  • crystal_1_11.bin
  • crystal_1_11.lib
  • crystal_1_12
  • crystal_1_12.bin
  • crystal_1_12.lib

@donovanglover
Copy link
Member

Seems to fail during checkPhase from the following:

In lib/markd/src/markd/renderers/html_renderer.cr:238:7

 238 | {% if Crystal::VERSION < "1.2.0" %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_140735611328032'

Called macro defined in lib/markd/src/markd/renderers/html_renderer.cr:238:7

 238 | {% if Crystal::VERSION < "1.2.0" %}

Which expanded to:

   1 | 
 > 2 |         title = URI.encode(node.first_child.text)
   3 |         @output_io << %(<a id="anchor-) << title << %(" class="anchor" href="#anchor-) << title << %("></a>)
   4 |       
Warning: Deprecated URI.encode. Use `.encode_path` instead.

A total of 1 warnings were found.

Could add doCheck = false; since it'd also reduce the build time a bit.

@donovanglover donovanglover added the 6.topic: crystal Programming language - https://crystal-lang.org/ label Oct 20, 2024
@straight-shoota
Copy link

Warnings should really not break a build. They're just warnings.

@donovanglover
Copy link
Member

Seems to be caused by make: *** [Makefile:110: compiler_spec] Illegal instruction (core dumped) later in the log

@will
Copy link
Contributor Author

will commented Oct 23, 2024

@donovanglover thanks so much for taking the time to check this!

I just got ahold of an x86 linux VM and am seeing if I can figure out what's up

@donovanglover
Copy link
Member

FWIW I'm okay with disabling checkPhase if Crystal still works. Not sure how hard it is to fix the tests.

@will
Copy link
Contributor Author

will commented Oct 25, 2024

FWIW I'm okay with disabling checkPhase if Crystal still works. Not sure how hard it is to fix the tests.

Oh okay, I pushed up a commit with doCheck false, and everything else built fine:

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review


x86_64-linux

⏩ 1 package marked as broken and skipped:
  • tijolo
✅ 21 packages built:
  • ameba
  • amqpcat
  • blahaj
  • collision
  • crystal
  • crystal.bin
  • crystal.lib
  • crystal2nix
  • crystalline
  • gi-crystal
  • icr
  • invidious
  • kakoune-cr
  • lucky-cli
  • mint
  • oq
  • rtfm
  • scry
  • shards
  • thicket
  • tmuxPlugins.fingers

I'm working with @straight-shoota to figure out the real problem, but I've been at a conference this week so haven't been able to dive in too deep yet, just have been able to reproduce it. He's on the core team for crystal so I think we'll be able to get to the bottom of it eventually

Copy link
Member

@donovanglover donovanglover left a comment

Choose a reason for hiding this comment

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

LGTM

@donovanglover donovanglover added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Oct 26, 2024
@donovanglover donovanglover merged commit d31617b into NixOS:master Oct 27, 2024
29 of 30 checks passed
@will
Copy link
Contributor Author

will commented Oct 27, 2024

Thanks @donovanglover for all the time and help with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: crystal Programming language - https://crystal-lang.org/ 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants