Skip to content

Conversation

@modocache
Copy link
Contributor

What's in this pull request?

build-script-impl currently maintains a list of NATIVE_TOOLS_DEPLOYMENT_TARGETS -- host machine targets, for which the resulting binaries can be run on the current machine.

However, there is only ever one host machine. This commit:

  • Changes the NATIVE_TOOLS_DEPLOYMENT_TARGETS list parameter into a single string parameter, HOST_TARGET.
  • Promotes the logic to detect the host target to Python, and places it in the swift_build_support module.
  • Removes the hard-coded "macosx_x86_64" path to the LLVM and Clang TableGen -- thereby unblocking future work to make cross-compilation possible on platforms other than OS X.
  • Also promotes cross-compilation target validation to Python, placing it in the swift_build_support module.

Why merge this pull request?

  1. It continues work on SR-237 by pulling more of the build-script-impl logic out of the shellscript and into Python.
  2. It removes hardcoded "macosx-x86-64" parameters from the build script. This is useful if we ever want the build script to allow cross-compilation from host machines other than OS X--which I think is indeed something we do want in the near future.

What downsides are there to merging this pull request?

  • It changes NATIVE_TOOLS_DEPLOYMENT_TARGETS from a list to a single element. I don't think there is any need multiple host targets, but I may be misunderstanding something.

@gparker42 gparker42 closed this Jan 13, 2016
@gparker42 gparker42 reopened this Jan 13, 2016
@gparker42
Copy link
Contributor

The only case for multiple host targets that I can think of is simultaneous 32- and 64-bit support. I don't think we use that anywhere today (in particular, Swift doesn't support 32-bit OS X). If we do need it we can probably say 64-bit is native and 32-bit is cross or something.

@modocache
Copy link
Contributor Author

The only case for multiple host targets that I can think of is simultaneous 32- and 64-bit support.

Aha, of course! Thanks for pointing that out.

If we do need it we can probably say 64-bit is native and 32-bit is cross or something.

Agreed. I'm hoping to expand the concept of "compilation targets" in upcoming pull requests, particularly to support cross-compilation from OS X to Linux ARM.

@gribozavr
Copy link
Contributor

Right, parallel 32 and 64 bit Linux builds was the motivation for using a list here.

@modocache modocache force-pushed the build-script-host-target branch 2 times, most recently from 22d143f to 2411f6d Compare January 13, 2016 18:44
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also noticed this hard-coded path, so I amended the commit to remove it as well.

@modocache
Copy link
Contributor Author

@gribozavr Do you think this is a reasonable approach for now? Or would you prefer something that would make it easier to add parallel 32- and 64-bit builds in the future?

@modocache modocache force-pushed the build-script-host-target branch 3 times, most recently from 53276a1 to ca0f7ea Compare January 19, 2016 16:22
@modocache
Copy link
Contributor Author

@gribozavr Friendly ping! I'd greatly appreciate feedback on what I could change here, or whether this isn't something you'd be interested in merging. 🙇

@modocache modocache force-pushed the build-script-host-target branch from ca0f7ea to 8ec280d Compare January 20, 2016 14:59
@tkremenek tkremenek assigned tkremenek and gribozavr and unassigned tkremenek Jan 24, 2016
@tkremenek
Copy link
Member

@gribozavr Any other feedback? (Aside from this change no longer merges cleanly)

@gribozavr
Copy link
Contributor

@modocache Sorry for the delayed response! The patch LGTM, please rebase to the current master, and I'll merge, thank you!

`build-script-impl` currently maintains a list of
`NATIVE_TOOLS_DEPLOYMENT_TARGETS` -- host machine targets, for which
the resulting binaries can be run on the current machine.

However, there is only ever *one* host machine. This commit:

- Changes the `NATIVE_TOOLS_DEPLOYMENT_TARGETS` list parameter into a
  single string parameter, `HOST_TARGET`.
- Promotes the logic to detect the host target to Python, and places it
  in the `swift_build_support` module.
- Removes the hard-coded "macosx_x86_64" path to the LLVM and Clang
  TableGen -- thereby unblocking future work to make cross-compilation
  possible on platforms other than OS X.
- Also promotes cross-compilation target validation to Python, placing
  it in the `swift_build_support` module.
@modocache modocache force-pushed the build-script-host-target branch from 8ec280d to c1a746b Compare January 24, 2016 16:59
@modocache
Copy link
Contributor Author

@gribozavr Not at all! I'm grateful for the review. I've updated and did another local test run; this appears to be working fine and is ready to be merged.

@gribozavr
Copy link
Contributor

Thank you!

gribozavr added a commit that referenced this pull request Jan 25, 2016
[build-script] There can only be one host target
@gribozavr gribozavr merged commit f6adbb6 into swiftlang:master Jan 25, 2016
@modocache modocache deleted the build-script-host-target branch January 25, 2016 01:13
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.

4 participants