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.

Sign up for GitHub

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

Jump to bottom

[build] avoid use generator expression to add linker parameter #29636

Merged
merged 2 commits into from
Feb 5, 2020
Merged

[build] avoid use generator expression to add linker parameter #29636

merged 2 commits into from
Feb 5, 2020

Conversation

edymtt
Copy link
Contributor

@edymtt edymtt commented Feb 4, 2020

Generator expressions seem not to allow the LINKER: prefix to be
expanded correctly when used in target_link_options.

To solve this, undo the change from #29451 and instead
use an if statement to add the flag when needed.

Addresses rdar://problem/59117166

@edymtt
[build] avoid use generator expression to add linker parameter
c60c69c 
Generator expressions seem not to allow the `LINKER:` prefix to be
expanded correctly when used in `target_link_options`.

To solve this, undo the change from swiftlang#29451 and instead
use an `if` statement to add the flag when needed.

Addresses rdar://problem/59117166
@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

@swift-ci please test

@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

@swift-ci please test Windows platform

@swift-ci
Copy link
Contributor

swift-ci commented Feb 4, 2020

Build failed
Swift Test OS X Platform
Git Sha - c60c69c

@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

The failure on Swift Test OS X Platform looks related to machine configuration

******************** TEST 'Swift(macosx-x86_64) :: Python/python_lint.swift' FAILED ********************
Script:
--
: 'RUN: at line 8';   /usr/bin/python /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/utils/python_lint.py
--
Exit Code: 1

Command Output (stdout):
--

The flake8 and flake8-import-order Python packages are required for linting,
but these were not found on your system.

You can install these using:

    python -m pip install flake8
    python -m pip install flake8-import-order

For more help, see http://flake8.pycqa.org.


--

@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

@swift-ci please test macOS

@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

@swift-ci please smoke-test

@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

@swift-ci please smoke test

@dan-zheng dan-zheng mentioned this pull request Feb 4, 2020
Merged
@compnerd
Copy link
Member

compnerd commented Feb 4, 2020

It is supported, but I think that it could be a limitation of 3.15. Could you please update the comment or better yet, add it as:

  if(CMAKE_VERSION VERSION_LESS 3.16)
    ...
  else()
    ...
  endif()

@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

@swift-ci please test

@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

@swift-ci please smoke test

@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

@swift-ci please test Windows platform

@swift-ci
Copy link
Contributor

swift-ci commented Feb 4, 2020

Build failed
Swift Test Linux Platform
Git Sha - c60c69c

@edymtt
Copy link
Contributor Author

edymtt commented Feb 4, 2020

Committed e438ce7 with the proposal -- we will introduce a bit of code duplication, but (a) both code paths are next to each other and (b) we will not forget about the opportunity to make this code more streamlined

@swift-ci
Copy link
Contributor

swift-ci commented Feb 4, 2020

Build failed
Swift Test OS X Platform
Git Sha - c60c69c

@edymtt
Copy link
Contributor Author

edymtt commented Feb 5, 2020

@swift-ci please test Windows platform

compnerd
compnerd approved these changes Feb 5, 2020
View reviewed changes
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.

Would be nice to squash these before merging

@edymtt edymtt merged commit 8a76358 into swiftlang:master Feb 5, 2020
edymtt added a commit to edymtt/swift that referenced this pull request Mar 10, 2020
@edymtt
[build] set linker parameters in a single way
77a6217 
After noticing that also in CMake 3.16 the LINKER: prefix is not
expanded correctly when used in `target_link_options`, prefer to set the
linker parameters in a more verbose way and leave a comment behind on
when this behavior was observed in case we want to change the
implementation later.

Follow up to swiftlang#29636.
Addresses rdar://problem/59732421
@edymtt edymtt mentioned this pull request Mar 10, 2020
Merged
edymtt added a commit that referenced this pull request Mar 11, 2020
@edymtt
[build] set linker parameters in a single way (#30339)
b3e8661 
After noticing that also in CMake 3.16 the LINKER: prefix is not
expanded correctly when used in `target_link_options`, prefer to set the
linker parameters in a more verbose way and leave a comment behind on
when this behavior was observed in case we want to change the
implementation later.

Follow up to #29636.
Addresses rdar://problem/59732421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd approved these changes

@shahmishal shahmishal Awaiting requested review from shahmishal

@clackary clackary Awaiting requested review from clackary

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edymtt @swift-ci @compnerd