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

Update shell.nix to nixpkgs-24.05 and LLVM 18 #14651

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Jun 1, 2024

Updates nixpkgs in shell.nix to 24.05 release. This release also brings LLVM 18 (and drops LLVM 11) so we update LLVM as well.

Closes #14269, #14592

@straight-shoota
Copy link
Member Author

Like in #14592 there are some spec failures in CI related to iconv.
Somebody should investigate that on a macOS machine.

@straight-shoota straight-shoota marked this pull request as draft June 7, 2024 12:21
@HertzDevil
Copy link
Contributor

HertzDevil commented Jun 10, 2024

Apparently Apple replaced the system iconv implementation from an old GNU libiconv to a FreeBSD-based one in 2023, so this is why I recalled not seeing those spec failures on my M2 in the past?

The actual bug is somewhat weird, and I have managed to reproduce it in C as well. String#encode uses an output buffer of 1024 bytes; if a single call to LibC.iconv fails because the output buffer has some space but is insufficient for the next character, then outbuf_ptr and outbytesleft only advance in increments of 32 bytes, even though the correct output bytes are written after outbuf_ptr for the input bytes it can consume. Those extra output bytes are then lost:

# first call consumes 1023 bytes and outputs 992 bytes; all the `y`s are lost
# second call consumes 3 bytes and outputs 2 bytes for the `好`
("x" * 992 + "y" * 31 + "").encode("EUC-JP").size # => 994

@HertzDevil
Copy link
Contributor

HertzDevil commented Jul 29, 2024

We should pass either -Dwithout_encoding-Dwithout_iconv or -Duse_libiconv. The latter requires pkgs.libiconvReal rather than pkgs.libiconv, but there isn't an associated pkg-config file and the linker flag is only available in $NIX_LDFLAGS?

@crysbot
Copy link

crysbot commented Aug 22, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/cross-compiling-crystal-applications-part-2/7090/12

luislavena added a commit to luislavena/crystal-xbuild-container that referenced this pull request Aug 22, 2024
Detect if macOS is the compilation target and forces libiconv usage
instead of macOS built-in iconv as is broken since 2023.

Ref:
* crystal-lang/crystal#14651 (comment)
@HertzDevil
Copy link
Contributor

My bad, it's -Dwithout_iconv rather than -Dwithout_encoding

@straight-shoota
Copy link
Member Author

Now for some reason CI on macOS doesn't even complete anymore. https://github.com/crystal-lang/crystal/actions/runs/11682308922/job/32535751549?pr=14651 is running for 4 hours now.

@HertzDevil
Copy link
Contributor

I think I have seen that outside this PR (https://github.com/crystal-lang/crystal/actions/runs/11856091183/job/33041700399), so I think we're fine?

@straight-shoota
Copy link
Member Author

Without completion of the CI job we cannot be sure the changes in this PR actually work as expected.
We can of course test it locally to give some confidence, but CI is a different environment.

Also the fact that this hangup reproduces consistently makes me think it is related to the change.

@HertzDevil
Copy link
Contributor

I tried applying #15150 and I think CI is progressing now: https://github.com/HertzDevil/crystal/actions/runs/11893760288/job/33139453764

@straight-shoota straight-shoota marked this pull request as ready for review November 19, 2024 11:55
@straight-shoota straight-shoota changed the title Update shell.nix to nixpkgs-24.05 Update shell.nix to nixpkgs-24.05 and LLVM 18 Nov 19, 2024
@straight-shoota
Copy link
Member Author

straight-shoota commented Nov 19, 2024

@HertzDevil That seems to have resolved it. So this is ready for review now.

We can probably update to nixpkgs 24.11 soon.

@straight-shoota straight-shoota added this to the 1.15.0 milestone Nov 19, 2024
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.

4 participants