Fix generated cc command for cross compile #13661
Conversation
Blacksmoke16
added
kind:bug
topic:compiler:codegen
kind:regression
fnordfish
changed the title
|
Just realized, that the |
There was a problem hiding this comment.
This looks like a good fix and the test is great to avoid repetition 🙇
There might be some merit in refactoring the code to clearly separate the different output path names by their function. But this is just a thought for later and out of scope for this bug fix.
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
spec/compiler/compiler_spec.cr
Outdated
| FileUtils.mkdir_p("#{path}/bin") | ||
| expected_bin = "#{path}/bin/compiler_sample-linux-x86_64" | ||
| output = IO::Memory.new | ||
| Process.run( |
There was a problem hiding this comment.
This is effectively an integration spec and will fail if a local Crystal compiler isn't built, as the spec will simply pick up the existing Crystal installation. You have two choices here:
- If you could get hold of a
Crystal::Compilerin this spec, it has a#stdoutproperty which you could reassign the aboveIO::Memoryto. Then you could callCrystal::Command.rundirectly, which uses the fresh compiler code, and test the compiler output as usual. The easiest way would probably be adding a newstdoutparameter inCommand#initializeand using it in#create_compiler. - Move this spec into a separate test suite, say
spec/compiler/integration_spec.cr, and create a corresponding new Makefile target that depends on the fresh compiler, similar to how$(O)/primitives_specdepends on$(O)/crystal. All the CI workflows must be updated to run this. This spec alone should be fast to build (i.e. it never requires the compiler source).
There was a problem hiding this comment.
Hm, isn't this the same with the following example?
Anywawy, I'd be happy to merge only the bugfix directly and figure out the spec later.
There was a problem hiding this comment.
This is true, that spec can also fail in the same way
There was a problem hiding this comment.
For the record, I think a unit test would be best for this. It may even need some refactoring to make that easier.
But we really just need a method that produces the linker command based on some pathnames and a basically empty Program instance with some flags configured.
|
@fnordfish In order to prioritize on shipping the bug fix, I think it's most practical to drop the spec from this PR. That way we can merge the bug fix right away for the release of 1.9.1. |
|
@straight-shoota Works for me. Should I remove the spec and squash everything down to a single commit? |
|
No need to squash or rebase, just a plain commit that reverts the spec |
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
Ref #13657