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.

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

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

Open
gottesmm opened this issue Dec 15, 2015 · 23 comments
Open

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

gottesmm opened this issue Dec 15, 2015 · 23 comments
Labels
build-script Area → utils: The build script compiler The Swift compiler in itself improvement

Comments

@gottesmm
Copy link
Member

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:

  • SR-9031 Make build-script multiplatform

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
Copy link
Contributor

+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.

@modocache
Copy link
Mannequin

modocache mannequin commented Dec 21, 2015

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.

@modocache
Copy link
Mannequin

modocache mannequin commented Dec 24, 2015

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
Copy link
Collaborator

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.

@modocache
Copy link
Mannequin

modocache mannequin commented Dec 29, 2015

rpinz (JIRA User): Awesome!

Just to give you some additional context on what (little) I've done so far, I also recently submitted #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
Copy link
Collaborator

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.

@modocache
Copy link
Mannequin

modocache mannequin commented Jan 20, 2016

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
Copy link
Collaborator

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
Copy link
Member

@modocache: Any update on this?

@modocache
Copy link
Mannequin

modocache mannequin commented Mar 26, 2016

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.

@rintaro
Copy link
Mannequin

rintaro mannequin commented Apr 7, 2016

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

@rintaro
Copy link
Mannequin

rintaro mannequin commented Apr 15, 2016

I posted a PR on #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

@rintaro
Copy link
Mannequin

rintaro mannequin commented Apr 25, 2016

PR #2190 was withdrawn.

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

Next action planned:

  • Migrate num_llvm_parallel_lto_link_jobs() and num_swift_parallel_lto_link_jobs() calculation

  • Migrate SWIFT_STDLIB_TARGETS, SWIFT_BENCHMARK_TARGETS, SWIFT_RUN_BENCHMARK_TARGETS, and SWIFT_TEST_TARGETS calculation

@ddunbar
Copy link
Member

ddunbar commented Jun 1, 2016

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.

@rintaro
Copy link
Mannequin

rintaro mannequin commented Jun 2, 2016

I don't have.
Current related PR in flight is
#2804

@ddunbar
Copy link
Member

ddunbar commented Jun 2, 2016

I posted the first part of one approach to allowing us to move the high-level control loop into `build-script`, here:
#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`.

@modocache
Copy link
Mannequin

modocache mannequin commented Jul 23, 2016

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
Copy link
Member Author

gottesmm commented Aug 4, 2016

I just added code in #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
Copy link
Member Author

gottesmm commented Aug 5, 2016

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
Copy link
Member Author

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.

@modocache
Copy link
Mannequin

modocache mannequin commented Aug 12, 2016

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

@MaxDesiatov
Copy link
Member

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
Copy link
Member

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

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@MaxDesiatov MaxDesiatov added the build-script Area → utils: The build script label Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-script Area → utils: The build script compiler The Swift compiler in itself improvement
Projects
None yet
Development

No branches or pull requests

6 participants