Add linker for MacOS Ventura (13.0.1) #702
Conversation
src/linker.rs
Outdated
| // otherwise need to match against every exact e.g. MacOS version | ||
| None => match target_os { | ||
| "linux" | "gnu" => Box::new(LdLinker::new()), | ||
| "darwin22.1.0" => Box::new(CcLinker::new("clang")), |
There was a problem hiding this comment.
The LD linker will work fine here and for now should be the default. Also if there's a way to avoid the version number it would be better maybe we can invert this Statement and only say windows does not get a linker. The idea was that Windows could not link using LD because we couldn't get it to accept all the correct settings at the time.
There was a problem hiding this comment.
Not sure why I didn't try LdLinker first but you're right, it's working :)
Anyways I don't have a Windows machine right now but can we assume target_os to always be windows or win32?
There was a problem hiding this comment.
windows and win32 are the only ones AFAIK
| @@ -290,8 +290,10 @@ fn link_missing_file() { | |||
| fs::remove_file(&out).unwrap(); | |||
| } | |||
|
|
|||
| // TODO: Ghaith please fix this :) | |||
There was a problem hiding this comment.
LD linker above should take care of this as far as I can tell
There was a problem hiding this comment.
Getting the following errors:
ld: warning: ignoring file /var/folders/f8/6xt81h0n473gvtqsbsr70mf00000gn/T/.tmpZ6EOgj/cc_proj.so, building for macOS-arm64 but attempting to link with file built for unknown-unsupported file format ( 0x7F 0x45 0x4C 0x46 0x02 0x01 0x01 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 )
and
thread 'integration::linking::link_to_a_relative_location_with_no_parent' panicked at 'called `Result::unwrap()` on an `Err` value: GeneralError { message: "\nlld: error: /var/folders/f8/6xt81h0n473gvtqsbsr70mf00000gn/T/.tmplCmy1R/output.o: unknown file type\n", err_no: linker__generic_error }', tests/integration/linking.rs:323:6
There was a problem hiding this comment.
Unrelated as we decided to use LD but when using clang the following error is yielded:
Undefined symbols for architecture arm64:
"_main", referenced from:
implicit entry/start for main executable
ld: symbol(s) not found for architecture arm64
clang-13: error: linker command failed with exit code 1 (use -v to see invocation)
There was a problem hiding this comment.
Ok, i'll take a look soon.
The LD linker is not supposed to be able to generate an executable unless you have a correct entry point.
The current advantage of of the LD linker is that it's built in/ embedded while clang is not.
Clang is however a compiler driver and thus makes it easier to run on different plafroms and to generate executables.
The error you are getting seems to be some missing libs in clang, not sure how that happened however.
For LD i think it compiled for the wrong target, is there a target in the test?
There was a problem hiding this comment.
For LD i think it compiled for the wrong target, is there a target in the test?
Yes, there's a global TARGET variable but it's not used for that specific test.
|
Also do we want to remove this hardcoded line or should we leave it for now? Lines 182 to 184 in be867c6 |
Yes we should not need this anymore i think |
- Remove `get_platform()` function as it's not used outside the `linker` module - Make linking possible for every platform but Windows, see also: https://github.com/PLC-lang/rusty/pull/702/files#r1052553988
Codecov ReportBase: 94.41% // Head: 94.43% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #702 +/- ##
==========================================
+ Coverage 94.41% 94.43% +0.01%
==========================================
Files 46 46
Lines 16228 16232 +4
==========================================
+ Hits 15321 15328 +7
+ Misses 907 904 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@ghaith would you be OK with the current solution, ignoring that one specific test for now? I'll see if I can fix this test in the coming days but merging this would be awesome because I could work on my MacBook without any local patching shenanigans :P |
|
Yes finde by me |
No description provided.