[build-script] Rename is_build_script_impl_product to needs_toolchain. #23918
Conversation
utils/swift_build_support/swift_build_support/products/product.py
Outdated
Show resolved
Hide resolved
drodriguez
force-pushed
the
build-script-rename-is-build-script-impl-product
branch
from
b8c199a to
8067ab7
Compare
|
Fix the typo and rebased on top of master to fix the conflicts. |
drodriguez
force-pushed
the
build-script-rename-is-build-script-impl-product
branch
from
8067ab7 to
9ea59a6
Compare
|
It might have used a toolchain from the previous build and I didn’t noticed. It was indeed wrong. Thanks for noticing. Fixed. Hopefully without blunders this time. |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test Linux |
|
Build failed |
|
The Linux failure seems to also be happening in the Android builds since a couple of builds ago. I haven’t have time to check what’s about, but it is something already in master (https://ci-external.swift.org/job/oss-swift-RA-linux-ubuntu-16.04-android-arm64/399/) |
|
Seems that the Linux problem has been disabled in Linux: #23974 |
drodriguez
force-pushed
the
build-script-rename-is-build-script-impl-product
branch
from
9ea59a6 to
1130626
Compare
|
@swift-ci please smoke test |
| @@ -20,8 +20,8 @@ def product_source_name(cls): | |||
| return "sourcekit-lsp" | |||
|
|
|||
| @classmethod | |||
| def is_build_script_impl_product(cls): | |||
| return False | |||
| def needs_toolchain(cls): | |||
There was a problem hiding this comment.
This is rather confusing name - pretty much anything compiled needs a toolchain. It seems that this means if we need the just built toolchain?
There was a problem hiding this comment.
Thanks for the feedback on the name. I don’t think there will be a good name, that it is also brief. When (If?) all my plans for a newer Python build script happens, I was planning to implement a simple dependency system to avoid this kind of booleans.
This “needs toolchain” means that the product needs the just created (and installed) toolchain in the installation directory. I’m not talking about any external toolchain. To avoid passing many parameters, IndexStoreDB and SourceKitLSP relies on an installed Swift toolchain (with Foundation and dispatch on them).
The problem with the previous name is that it was going start lying as soon as the products get turned from build-script-impl into Python.
I accept any proposals. Maybe needs_swift_toolchain or needs_installed_swift_toolchain are better options?
There was a problem hiding this comment.
@compnerd some idea which name will be better according to you? Any name works for me, as long as it doesn't refer to “build script impl”, since I don’t think it is related to it (anymore).
drodriguez
force-pushed
the
build-script-rename-is-build-script-impl-product
branch
from
1130626 to
7da669d
Compare
While the previous name is factual at the moment, I think needs_toolchain describes better why those products are special. Sadly Ninja has to be always a special product, and it doesn't fit needs_toolchain, but it also doesn't need to be handled with the other products, so answering False for it is fine. The rest of the changes are mechanical, and flipping the logic around.
drodriguez
force-pushed
the
build-script-rename-is-build-script-impl-product
branch
from
7da669d to
68c7e41
Compare
|
Build failed |
|
Build failed |
|
Abandoning these efforts. It might still be useful for better Windows builds for most of the community, but I will not have time to keep improving this and it doesn't seem to be a lot of interest in improving the build system in this direction. |
While the previous name is factual at the moment, I think needs_toolchain
describes better why those products are special.
Sadly Ninja has to be always a special product, and it doesn't fit
needs_toolchain, but it also doesn't need to be handled with the other
products, so answering False for it is fine.
The rest of the changes are mechanical, and flipping the logic around.
With all the work I'm trying to do to remove
build-script-impl, the name of the property was always bugging me, because it will lie the moment I start implementing the first builders that bypassbuild-script-impl. When I saw #23849 I found that Michael has found the perfect replacement: all this product need a toolchain to build. This change will allow to keep the name during the transition and will be less confusing.This was part of #23257. I’m trying to split the PR in smaller ones to make the reviews easier. Other PRs in this group are #23303, #23803, #23810, #23822, #23865, #23915 and #23917.