-
Notifications
You must be signed in to change notification settings - Fork 69
[native_toolchain_c] Add linking for macOS #2360
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
Conversation
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
The breaking check still has problems. I verified locally that this isn't a breaking change. |
I've filed #2361. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @goderbauer !
) { | ||
final includeAllSymbols = _symbolsToKeep == null; | ||
|
||
if (targetOS == OS.macOS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be written as a switch on target OS (which then has both the Android and Linux cases got to the same block). Early return for MacOS and combining all other OSes seems somewhat weird.
}) async { | ||
assert((targetOS != OS.android) == (androidTargetNdkApi == null)); | ||
assert((targetOS != OS.macOS) == (macOSTargetVersion == null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider throwing argument errors instead of doing asserts?
Especially because below we check macOSTargetVersion != null
instead of targetOS == OS.macOS
.
// See https://github.com/dart-lang/native/issues/1376. | ||
@TestOn('linux') | ||
@TestOn('linux || mac-os') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Hm, they should have made a bunch of const objects and an Or([Linux(), MacOS()])
class instead of going for strings. How do we know if this syntax is correct and we're not just skipping the test everywhere because of wrong syntax. 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was paranoid about that as well and double-checked that the test runs. :D
Towards #1376
This enables linking for MacOS targets on MacOS hosts.