Closed kalinon closed 4 years ago
These specs are still broken:
Failed examples:
crystal spec spec/integration/containers_spec.cr:4 # container instantiation feature works
crystal spec spec/integration/arguments_spec.cr:4 # the argument translation functionality works
Hey will spec/integration/spec_helper.cr
somehow use llvm-config
that is discovered, or it will assume the use of llvm-config
which is currently in PATH?
Right now its whats currently in PATH
unsure the best way to go about it. I hate running cmds to gather info.
Yes, but I'm thinking that we should make it use the settings discovered by / saved into Makefile.variables
. This would avoid running commands, and it'd also be the right value to use. I am looking into it as we speak, will report if I have a specific idea how to reuse.
In fact, shouldn't find_clang.cr
be in charge of generating spec_base.yml
when it runs as well, in addition to Makefile.variables
? It is all the same data.
Let me know if you'd want me to make these adjustments. Then afterwards we could also rename the clang/
directory and find_clang.cr
script to some other names that better represent what they contain/do.
I also thought about that. I am ok with it, i was also re-evaluating the find_clang.cr
in my head, as it really can be replaced by llvm-config
we do a lot of extra processing and file discovery for what essentially the following llvm-config
commands provide:
Yes, good that you mention it, for example, to simplify we could also always use the compiler and llvm-config which is in the current PATH. This way, the user is responsible for adjusting the environment before running the build, and we don't do any autodetection. (We may check if the version selected is supported etc., but we wouldn't do any auto-discovery of binaries or versions.)
Agreed. Go for it!
I am looking at Stefan's comments in find_clang.cr
and there is:
# Find llvm config if we are using llvm
llvm_config_binary = find_llvm_config_binary
Is it even possible that llvm is not used?
Hey @kalinon I've committed the changes integrating this with some general cleanup in find_clang.cr.
There is still some overlap between CMake and find_clang.cr. (When CMake finds the clang binary, then any/all autodetection in find_clang.cr becomes both unnecessary as well as wrong, since if CMake and find_clang don't agree on the binary it's going to cause a problem later on.)
However, this will not be an issue in typical builds because if --clang PATH
is given (and it always is when invoked from cmake/make), then its autodetection is disabled. So the only drawback to leaving autodetection to CMake is that probably the autodetection in find_clang.cr is more verbose than one in CMake (for example it can search for patterns like "clang++-*" which would find/match clang++-7
and so on.)
In any case I think it's a good readability/simplicity improvement that can be refined even more over one or two future iterations.
I will close this PR. Please report if what is currently in master causes any issues.
Also related to #28 , this change is to auto generate the
spec_base.yml
for integration tests. This will use thellvm-config
command to populate as many fields as it can.Example output of file with llvm 10