[build-script] Add extra-stdlib-deployment-targets to build-util #13106
Conversation
|
@jrose-apple @gparker42 @gottesmm also split off off from #12955 PR and should be good to land on it's own. |
|
@Rostepher ping |
|
This is so I can build a darwin toolchain, with all the implicit targets that come by default when you don't set stdlib-deployment-targets (like macos, iOS, watch, tv, etc) for each supported arch, and then just tack on my "fuchsia-x86_64,fuchsia-aarch64,android-armv7" target archs. |
There was a problem hiding this comment.
Overall this looks pretty good.
I can see the need for this sort of flag, but would it be enough to consider multiple uses of --stdlib-deployment-targets? It can be called multiple times to append more targets to build.
There's also some merge conflicts from #12954 which will need to be resolved.
| help="list of additional targets to cross-compile the Swift standard " | ||
| "library for in addition to default", | ||
| action=arguments.action.concat, type=arguments.type.shell_split, | ||
| default=None) |
There was a problem hiding this comment.
If this is meant as a list of deployment targets, why not make the default an empty list. I plan to change the --stdlib-deployment-targets option to do the same.
There was a problem hiding this comment.
If possible would you mind also following the style changes from #12954, namely putting the help string last and action first in the list of kwargs.
There was a problem hiding this comment.
Yeah empty list would work too.
| @@ -507,6 +507,7 @@ class IgnoreOption(_BaseOption): | |||
| AppendOption('--extra-cmake-options'), | |||
| AppendOption('--extra-swift-args'), | |||
| AppendOption('--stdlib-deployment-targets'), | |||
| AppendOption('--extra-stdlib-deployment-targets'), | |||
There was a problem hiding this comment.
👍 Thanks for adding this to the test suite!
Rostepher
changed the title
|
Yeah, it's not wanting to call stdlib-deployment-targets multiple times but I just want to append to the default implicit targets that build-util setups by default for the current OS that you get when you don't set it in the first place. For Linux it's only one arg but for Darwin it ends up being more than dozen (iOS-arm64, watch-arm64, macOS-x86_64, etc). I just don't want to recreate them all and keep the list up to date in our CI builder's ini. :) |
zbowling
force-pushed
the
extra-stdlib
branch
from
1b4b562 to
34517dc
Compare
|
Updated to the options system and rebased on master. Please review. |
|
@swift-ci please test |
|
@swift-ci please smoke test |
|
Build failed |
|
Build failed |
zbowling
force-pushed
the
extra-stdlib
branch
from
34517dc to
82ece2d
Compare
zbowling
force-pushed
the
extra-stdlib
branch
from
82ece2d to
00d8381
Compare
|
fixed the expected default value that broke the test. should be good to test. |
| type=argparse.ShellSplitType(), | ||
| default=None, | ||
| help="list of additional targets to cross-compile the Swift standard " | ||
| "library for in addition to default") |
There was a problem hiding this comment.
This will fail the Python linter test. Please indent put to the above quote and please use single quotes for string literals.
| @@ -585,6 +585,12 @@ def create_argument_parser(): | |||
| help='list of targets to compile or cross-compile the Swift ' | |||
| 'standard library for. %(default)s by default.') | |||
|
|
|||
| option('--extra-stdlib-deployment-targets', append, | |||
| type=argparse.ShellSplitType(), | |||
| default=None, | |||
There was a problem hiding this comment.
Would you mind making the default an empty list? Then you wouldn't need this explicit line at all.
|
The review comments here were never addressed, and it's been a few years. If you'd like to pursue this change, please rebase this patch and address @Rostepher's comments. |
Adds a new
--extra-stdlib-deployment-targetsarg that appends insteads of replaces the default archs to build the stdlib lib for, which can be an extensive list, especially on Darwin, to recreate.Split from of #12955