-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Reorder swiftc args to allow user override #715
Conversation
LGTM, can we get a functional test that this solves the problem (on macOS only, probably)? |
Looks good as a short term workaround. Thanks! |
should this be done for C targets too? |
Probably. I can add that to this PR if you'd like. |
Thanks! |
Did you want to add the C target diff to this PR too, or is there any use in merging it separately? If so, we should have a JIRA covering the C case. |
I've added the C target change here. But haven't dove in to how to add tests for this yet. I'll try and tackle that shortly. |
Great! what you can do for tests is, create a fixture that would only build if this patch is present and drop it in
Let me know if you need any help figuring stuff out! |
I added a small test (which feels pretty brittle). Feedback appreciated! |
LGTM, I don't know if there is a better to way to test this (right now) |
Currently the build target arguments are hard coded in the swiftpm source. Until they are broken out into some configuration method, it would be useful for users to be able to override these arguments for their use case. This change reorders the passed arguments to pass along the users arguments after the default arguments.
This tests our ability to pass a custom minimum deployment target as a swiftc argument in order to deploy to a higher version than what is currently checked in as the minimum in the swiftpm source.
Updated the test to use a custom function with a higher deployment target instead of something from foundation. I think this should be less brittle. |
Thanks for the test, new version looks very reasonable to me. LGTM! |
@swift-ci please test |
/// This file exists to test the ability to override deployment targets via args passed to swiftc | ||
/// For this test to work, this file must have an API call which was introduced in a version | ||
/// higher than the default macOS deployment target that is checked in. | ||
@available(macOS 10.20, *) |
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.
Cool, I was wondering if available API could be used
@swift-ci please test |
Thanks! |
This fixes https://bugs.swift.org/browse/SR-2535 |
Looks good to me as well. Thanks! |
Currently the build target arguments are hard coded in the swiftpm
source. Until they are broken out into some configuration method, it
would be useful for users to be able to override these arguments for
their use case. This change reorders the passed arguments to pass along
the users arguments after the default arguments.
See this thread for more info: https://lists.swift.org/pipermail/swift-build-dev//Week-of-Mon-20160919/000637.html