search

swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.58k stars 10.36k forks source link

[SR-237] Merge build-script-impl into build-script #42859

Open gottesmm opened 8 years ago

gottesmm commented 8 years ago
Previous ID SR-237
Radar None
Original Reporter @gottesmm
Type Improvement
Status Reopened
Resolution
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 5 | |Component/s | Compiler | |Labels | Improvement | |Assignee | None | |Priority | Medium | md5: 4ed4f6ff1e049872fedd73313bcf3878

relates to:

Issue Description:

Historically, the reason why there is build-script and build-script-impl is that original build-script-impl was build-script but we needed a better interface on top that suggested we wanted something in python (ala build-script), not bash (ala build-script-impl).

One thing that has been on the back burner for a while is to port build-script-impl to python and merge it into build-script so we only have one build-script.

belkadan commented 8 years ago

+1. Note that any work here should start by preserving all the existing semantics of build-script-impl. We can work on stripping things out after that.

ec882e32-f2b6-4d2a-849c-98d6c7df0cfb commented 8 years ago

I agree that this would be an improvement. I think we should merge `utils/SwiftBuildSupport.py`, `utils/build-script`, and `utils/build-script-impl` into a Python package. The package structure would allow us to split the functionality out into separate files with single responsibilities.

ec882e32-f2b6-4d2a-849c-98d6c7df0cfb commented 8 years ago

I've begun working on this:

The second pull request begins work on my suggestion to move all build-related logic to its own Python module. Feedback welcome!

swift-ci commented 8 years ago

Comment by Ron Pinz (JIRA)

In order to help facilitate this endeavor I will be submitting pull requests that restructure and harden the 'utils/build-script-impl' script. The current state of the script is a patchwork of changes that at times approaches being difficult to follow and was likely a contributing factor to this decision.

ec882e32-f2b6-4d2a-849c-98d6c7df0cfb commented 8 years ago

rpinz (JIRA User): Awesome!

Just to give you some additional context on what (little) I've done so far, I also recently submitted https://github.com/apple/swift/pull/792. My strategy has been to move logic out of `build-script-impl` piece by piece, starting from the top of the file. In that vein, my next set of changes will involve moving setting the default value for CMAKE_INSTALL_PREFIX and setting the deployment target options.

Once the majority of the script is moved to Python, I'd love to use a spiffy abstraction to emit the CMake build command--perhaps a factory object that vends a CMake configuration, which is then responsible for translating its attributes into a build invocation?

configuration = CMakeConfiguration().installPrefix('/usr').deploymentTarget('linux-armv7')
subprocess.check_call(configuration.command)

Of course these are just some ideas I had, feel free to take whichever direction you think is best. Would love to collaborate if possible, though!

swift-ci commented 8 years ago

Comment by Jay Buffington (JIRA)

Since CentOS 6.x still ships with python 2.6, it would be nice to not use python 2.7 or higher features.

ec882e32-f2b6-4d2a-849c-98d6c7df0cfb commented 8 years ago

jaybuff (JIRA User) Oh wow, good point! I wonder if maybe we should track that in a dedicated issue--I'm sure many build scripts don't work on Python 2.6. For example, I'm pretty sure that non-indexed string formatting (that is, "{}".format(value), without a 0 in between the {}) isn't supported in Python 2.6, but if you git grep "{}.*".format", you'll find a ton of places it's being used.

I think opening a dedicated issue, or discussing it on the mailing list, is the best way to go. We'll also need to make sure that it's tested somehow--it's very easy for developers to break Python for versions of the interpreter that they don't use themselves.

swift-ci commented 8 years ago

Comment by Jay Buffington (JIRA)

Thanks @modocache! I noticed the format one as well as subprocess using check_output, which is a 2.7 feature.

lplarson commented 8 years ago

@modocache: Any update on this?

ec882e32-f2b6-4d2a-849c-98d6c7df0cfb commented 8 years ago

It's moving along very slowly, meanwhile things get added to build-script-impl little by little. I think of the migration process as being composed of the following steps:

  1. Move everything that isn't building/testing/installing out of build-script-impl. Things like creating a toolchain and running package tests on it, for example, should be moved to Python.

  2. Move installing into Python.

  3. Move testing into Python.

  4. Move building into Python.

I think ideally the Python script could use some sort of abstraction responsible for generating an array of shell commands necessary to do any of the above steps.

Feel free to grab the task from me--I'm interested in working on it but am busy with other things just this moment.

41412bb9-c79a-4f5a-8a7f-383cb40aa74b commented 8 years ago

I'm working on this, but it would be not so fast.

41412bb9-c79a-4f5a-8a7f-383cb40aa74b commented 8 years ago

I posted a PR on https://github.com/apple/swift/pull/2190

You can try it locally by:

swift$ git fetch origin pull/2190/head:SR-237-rewrite
swift$ git checkout SR-237-rewrite
swift$ utils-experimental/build-script -RT
41412bb9-c79a-4f5a-8a7f-383cb40aa74b commented 8 years ago

PR #2190 was withdrawn.

We are doing incremental migration.
After several gardening works, I've started migration:

Next action planned:

ddunbar commented 8 years ago

Does anyone have outstanding patches on this not reflected in current PRs? I would like to, maybe, contribute to this as part of my "Toolchain-based build process" proposal, but I don't want to do any work which is going to conflict with existing progress here.

41412bb9-c79a-4f5a-8a7f-383cb40aa74b commented 8 years ago

I don't have.
Current related PR in flight is
https://github.com/apple/swift/pull/2804

ddunbar commented 8 years ago

I posted the first part of one approach to allowing us to move the high-level control loop into `build-script`, here:
https://github.com/apple/swift/pull/2844

The basic idea is we would compute the high-level list of "actions" to perform in `build-script`, then invoke the `build-script-impl` once per action (in the right order). If we do that, it is then easy to incrementally move individual actions from the sequence to being entirely done via the `build-script`.

ec882e32-f2b6-4d2a-849c-98d6c7df0cfb commented 8 years ago

Various people have worked on this for half a year – I don't think it's a "starter bug", so I removed the label. 🙂

gottesmm commented 8 years ago

I just added code in https://github.com/apple/swift/pull/3965 to enable individual product cmake options to be ported to build-script. A large portion of build-script-impl involves these product specific arguments, so we should be able to thin build-script-impl considerably and remove a bunch of build-script-impl options.

gottesmm commented 8 years ago

Question to everyone watching this. Given that amount of work that everyone has done so far, I wonder if it makes sense to split this SR up into multiple SRs. I think some of them should be starter bugs such as thinning out build-script-impl via the option I just added... We could have migrating individual parts of the code into separate SRs...

Thoughts?

gottesmm commented 8 years ago

I have started to move some flags from build-script-impl => the product specific cmake support I added.

I am only going to do a few to provide examples for other people to use/copy/paste/etc.

ec882e32-f2b6-4d2a-849c-98d6c7df0cfb commented 8 years ago

Sorry for the delayed response. An enthusiastic +1 from me to split this up into several smaller tasks. Thanks, @gottesmm!!

MaxDesiatov commented 2 years ago

Hi d066z (JIRA User), I see this has been resolved, but build-script-impl is still present in the compiler repository. Would you be able to clarify the status of this? Isn't build-script-impl supposed to be removed after its functionality has been replaced by the Python build-script, or is my understanding of how this is supposed to work wrong?

MaxDesiatov commented 2 years ago

Reopening this as confirmed with one of the members of the Swift team. This hasn't been resolved yet.