-
Notifications
You must be signed in to change notification settings - Fork 126
Fix MacOS C++ tests compatibility issues #3677
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
base: master
Are you sure you want to change the base?
Changes from 11 commits
f1a5999
3f999c4
13405a6
d822e30
58d7e18
779d584
28fcbe4
e6ed708
793110c
031ab83
bbf6240
0dcc1c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,7 +120,13 @@ pyqtgraph==0.13.7 \ | |
| --hash=sha256:64f84f1935c6996d0e09b1ee66fe478a7771e3ca6f3aaa05f00f6e068321d9e3 \ | ||
| --hash=sha256:7754edbefb6c367fa0dfb176e2d0610da3ada20aa7a5318516c74af5fb72bf7a | ||
| # via -r software/thunderscope/requirements.in | ||
| qtawesome==1.4.0 \ | ||
| --hash=sha256:783e414d1317f3e978bf67ea8e8a1b1498bad9dbd305dec814027e3b50521be6 \ | ||
| --hash=sha256:a4d689fa071c595aa6184171ce1f0f847677cb8d2db45382c43129f1d72a3d93 | ||
| # via -r software/thunderscope/requirements.in | ||
|
Comment on lines
+123
to
+126
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these intended dep updates?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do they need to be done, seems to be something fit for another pr (or for #3542, I promise I'll finish that soon)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the software tests that failed was for the requirements; I just updated the requirements cause it was supposed to be done way earlier |
||
| qtpy==2.4.2 \ | ||
| --hash=sha256:5a696b1dd7a354cb330657da1d17c20c2190c72d4888ba923f8461da67aa1a1c \ | ||
| --hash=sha256:9d6ec91a587cc1495eaebd23130f7619afa5cdd34a277acb87735b4ad7c65156 | ||
| # via pyqt-toast-notification | ||
| # via | ||
| # pyqt-toast-notification | ||
| # qtawesome | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,6 +227,9 @@ def _construct_cc_info( | |
| user_compile_flags = copts, | ||
| ) | ||
|
|
||
| # Linker flags required for macos, doesn't affect linux | ||
| link_flags = ["-Wl,-undefined,dynamic_lookup"] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this affect the robots/cross-compile toolchain? They run on ARM.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll look into it a bit more, from what I understand its a difference between apple's ld and gnu ld
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea this is a linux vs macos linker issue; I've added an if statement that only passes these flags when running macos cause the -undefined flag is actually different in GNU ld |
||
|
|
||
| (linking_context, linking_outputs) = \ | ||
| cc_common.create_linking_context_from_compilation_outputs( | ||
| name = "link_nanopb_outputs", | ||
|
|
@@ -235,6 +238,7 @@ def _construct_cc_info( | |
| feature_configuration = feature_configuration, | ||
| cc_toolchain = cc_toolchain, | ||
| linking_contexts = nanopb_linking_contexts, | ||
| user_link_flags = link_flags, | ||
| ) | ||
|
|
||
| extra_context = cc_common.create_compilation_context( | ||
|
|
||
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.
This looks fine, but just curious:
Is there any reason that we did not use polyfills such as
https://github.com/adobe/lagrange/blob/771d85889dd052b545de1fa4a66fe4b3ff2c5e91/modules/core/src/fpe.cpp#L94-L113
Not sure if polyfills like this will work, but I think if it does, it will enable more consistency behaviours across different platforms?
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.
Oh yea I tested it and it should technically be possible since I got it working for a minimal example https://gist.github.com/nycrat/3bfd574a7b3d328e51ae1b3705de9ebc. But for some reason it doesn't work with gtest. I'll keep trying to make it work since that would be optimal
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.
Yea I can't seem to get it working, something with the gtest environment I guess?