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.

Sign up for GitHub

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

Jump to bottom

crystal: 0.23.0 -> 0.23.1 #29966

Merged
merged 3 commits into from Oct 4, 2017
Merged

crystal: 0.23.0 -> 0.23.1 #29966

merged 3 commits into from Oct 4, 2017

Conversation

david50407
Copy link
Member

Motivation for this change

Update the package to the latest version (0.23.1-3).

NOTE: Due to crystal-lang/crystal#4719, when building Crystal with LLVM 4 with debug infos from prebuilt binary (w/ LLVM 3.8) will always be failed.
The temporary solution is to build a LLVM 4 version without debug info, then use it to build Crystal with debug info on LLVM 4.
This will cause building phrase going longer than normal. But we can remove this hack after Crystal fix the issue.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@orivej
Copy link
Contributor

orivej commented Sep 30, 2017

Does it build with llvm_39 or llvm_38 in a single pass?

@david50407
Copy link
Member Author

@orivej No, still need twice on llvm_39

@sifmelcara
Copy link
Member

llvm_5 has been merged into master two days ago. Maybe we can try llvm_5 with llvm5 patch provided by upstream?

@orivej
Copy link
Contributor

orivej commented Oct 3, 2017 •
edited

This version of crystal does not support llvm5. The next thing I wanted to check is if the previous release of crystal could compile this release.

@sifmelcara
Copy link
Member

I think by applying the patch I mentioned above, this version of crystal can use llvm5, and this is what archlinux does currently.

@orivej
Copy link
Contributor

orivej commented Oct 3, 2017

This patch fixes adds support for LLVM 5 but does not fix the build:

Using /nix/store/5l8xwhsmngm9d7ama59a1f1z3s31f52y-llvm-5.0.0/bin/llvm-config [version=5.0.0]
g++ -c  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc `/nix/store/5l8xwhsmngm9d7ama59a1f1z3s31f52y-llvm-5.0.0/bin/llvm-config --cxxflags`
gcc -fPIC    -c -o src/ext/sigfault.o src/ext/sigfault.c
./bin/crystal doc src/docs_main.cr
--: 1: --: git: not found
ar -rcs src/ext/libcrystal.a src/ext/sigfault.o
./bin/crystal build --release  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
crystal: /var/cache/omnibus/src/llvm/llvm-3.8.1.src/lib/CodeGen/LexicalScopes.cpp:160: llvm::LexicalScope* llvm::LexicalScopes::getOrCreateRegularScope(const llvm::DILocalScope*): Assertion `cast<DISubprogram>(Scope)->describes(MF->getFunction())' failed.
/place/vartmp/nix-build-crystal-0.23.1-3.drv-0/crystal-0.23.1/../crystal-0.23.1-3/bin/crystal: line 102: 272951 Aborted                 "$INSTALL_DIR/embedded/bin/crystal" "$@"
make: *** [Makefile:115: .build/crystal] Error 134

@orivej
Copy link
Contributor

orivej commented Oct 3, 2017

The next thing I wanted to check is if the previous release of crystal could compile this release.

It works, I pushed the result to this pull request. Could you test it on macOS?

@sifmelcara
Copy link
Member

Oh, so I misunderstood the problem, what make archlinux successfully build 0.23.1 is make --no-debug, not llvm_5. (By the way, they use --no-debug for the reason other then this issue)

I did a quick testing to build crystal using old version of crystal ( https://gist.github.com/sifmelcara/c5d5b0670bf82693a5bdc9468bb16fb9 )
It builds, but the compiled binary seems to report wrong version...

@orivej
Copy link
Contributor

orivej commented Oct 3, 2017

It builds, but the compiled binary seems to report wrong version...

Indeed. That's ridiculous! Can you distinguish 0.23.0 from 0.23.1 not looking at the version number?

@sifmelcara
Copy link
Member

sifmelcara commented Oct 3, 2017 •
edited

crystal-lang/crystal#4687
I just found that crystal will try to find its version from .git directory, if there is no .git directory, it inherits the version of compiler used to compile itself.
So I guess we need to manually patch the compiler version info.

sifmelcara
sifmelcara reviewed Oct 3, 2017
View reviewed changes
pkgs/development/compilers/crystal/default.nix Outdated
'';

makeFlags = [ "release=1" "all" "doc" ];
Copy link
Member

Choose a reason for hiding this comment

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

Add "CRYSTAL_CONFIG_VERSION=${version}" to makeFlags seems to fix the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the investigation!

@orivej orivej force-pushed the patch-1 branch 2 times, most recently from 7540781 to b866890 Compare October 4, 2017 07:36
david50407 and others added 3 commits October 4, 2017 07:36
@david50407 @orivej
Add david50407 into maintainers.nix
d514aa5
@david50407 @orivej
crystal: 0.23.0 -> 0.23.1
bc2ac2a 
Due to crystal-lang/crystal#4719,
when building Crystal with LLVM 4 with debug infos from prebuilt binary (w/ LLVM 3.8) will always be failed.
The temporary solution is to build a LLVM 4 version without debug info,
then use it to build Crystal with debug info on LLVM 4.

This will cause building phrase going longer then normal.
We can remove this hack after Crystal fix the issue.
@orivej
crystal: fix build by using the previous bootstrap
766ede0
@orivej orivej merged commit 28bc4ef into NixOS:master Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orivej orivej orivej left review comments

@sifmelcara sifmelcara sifmelcara left review comments

@edolstra edolstra Awaiting requested review from edolstra

Assignees
No one assigned
Labels
12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@david50407 @orivej @sifmelcara @Janik-Haag