Skip to content
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

buildsystem: fix build for macOS #25

Merged
merged 3 commits into from
Jan 14, 2018
Merged

Conversation

tusharpm
Copy link
Member

  • use minimum required flex version 2.6 (Apple gives an older version built-in)
  • fix linker flag for Apple

- use minimum required flex version 2.6 (Apple gives an older version built-in)
- fix linker flag for Apple
@tusharpm tusharpm added improvement improves existing functionality buildsystem something with cmake labels Jan 13, 2018
@tusharpm tusharpm requested a review from TheJJ January 13, 2018 09:52
- the default treatment of `-undefined` is `error` for `ld` on Apple
Copy link

@nyx nyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I had locally just hardcoded the APPLE conditional here with intent to conditionalize later. However if you'd prefer to keep parity there (at least my issue was with Clang not accepting those flags) consider using something like this which I believe is equivalent:

+       # for Clang the options are different
+       set_target_properties(nyan PROPERTIES LINK_FLAGS "-Wl,-undefined,error")

@tusharpm
Copy link
Member Author

Thanks for the review. 👍

About the specification of the link flag: I have that in my first commit. However, in my opinion, these flags are better left as implicit defaults if they're favorable. On macOS, man ld states "[...] The default is to treat undefined symbols as errors."
I usually avoid specifying platform/tool-specific flags if possible. If the explicit specification solves some issue, I can remove my second commit (assuming the first one is acceptable, of course).

@nyx
Copy link

nyx commented Jan 13, 2018

@tusharpm OK - that's fine by me. It seems doubtful that the defaults there will change any time soon, and I favor minimalism. Maybe note that in a comment?

Copy link
Member

@TheJJ TheJJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should do.

@TheJJ TheJJ merged commit 342f5bb into SFTtech:master Jan 14, 2018
@tusharpm tusharpm deleted the macos_update branch January 14, 2018 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildsystem something with cmake improvement improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants