Skip to content

[CMake] Localize a build hack #12880

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

Closed

Conversation

davezarzycki
Copy link
Contributor

The '-Wl,-z,defs' flag is useful, so let's disable it only where needed.

The '-Wl,-z,defs' flag is useful, so let's disable it only where needed.
@davezarzycki
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 11060d4

@davezarzycki
Copy link
Contributor Author

The only failure in the full test was/is the known and unrelated ModelIO.swift failure. Let's smoke test this PR to bypass that problem and be merge ready.

@swift-ci please smoke test

@davezarzycki
Copy link
Contributor Author

@compnerd -- Does this patch look okay with you?

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Why do you suspect that this only is an issue for the SwiftRemoteMirror? I suspect that this breaks the tests too for Windows when cross compiling. I’d say if you want -z defs, you should do this at a much deeper layer in swift_add_library with ELF style output.

@davezarzycki
Copy link
Contributor Author

davezarzycki commented Nov 13, 2017

Hi @compnerd – A few thoughts and questions:

  1. Do we agree that "-Wl,-z,defs" is a good thing? As a reminder, it forces the linker to not ignore missing definitions/symbols.
  2. In my Linux ELF system testing, SwiftRemoteMirror is the only library that "needs" the flag removed, and I think even that case is fixable.
  3. I don't have access to a Windows machine, nor are Windows machines a part of Swift's continuous integration, so any advice that isn't speculative would be helpful. Would you be open to trying this patch out? We can always revert it if Windows needs the flag removed in more scenarios.
  4. At the very least, I'd like to make the top level removal of this flag conditional on platforms that need it. What do you think of this option?

@davezarzycki
Copy link
Contributor Author

On second thought, I'll just preserve the Windows behavior. Please consider reviewing #12895 instead. Thanks!

@davezarzycki davezarzycki deleted the cmake_localize_hack branch November 13, 2017 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants