[build] Disable cleaning sourcekit-lsp by default, rather than having the flag do nothing #75053
Conversation
…g, ie clean by default Instead, swiftlang/swift#75053 makes sure `build-script` passes in the flag by default, so you can explicitly override it when needed.
| @@ -85,7 +85,7 @@ def get_dependencies(cls): | |||
|
|
|||
|
|
|||
| def run_build_script_helper(action, host_target, product, args, | |||
| sanitize_all=False, clean=True): | |||
| sanitize_all=False, clean=False): | |||
There was a problem hiding this comment.
Won’t this break the indexstore-db build, which would now pass --no-clean, which isn’t accepted by the build-script-helper.py?
My proposal would be to just remove the cleaning step from SourceKit-LSP’s build-script-helper.py, never pass --no-clean and remove the clean parameter here.
There was a problem hiding this comment.
Won’t this break the indexstore-db build, which would now pass --no-clean, which isn’t accepted by the build-script-helper.py?
It might, haven't tried that recently.
My proposal would be to just remove the cleaning step from SourceKit-LSP’s build-script-helper.py, never pass --no-clean and remove the clean parameter here.
That would defeat my purpose of optionally being able to clean sourcekit-lsp by manually overriding these flags when wanted.
Do you not want that cleaning step at all in the sourcekit-lsp build-script-helper.py? I would prefer to resurrect it in some way, but disabled in these scripts by default.
Let me know which approach you want and I will change these two pulls to do that.
There was a problem hiding this comment.
I would prefer to not have the clean step as it prevents us from re-using build artifacts from eg. the build phase in the test phase, which increases build time. If cleaning is required between builds, I think it should be done on a higher level.
There was a problem hiding this comment.
I was asking a different question, I think. I see two options here:
- Remove all code related to cleaning
sourcekit-lspbuilds. - Bring back the cleaning code in the
sourcekit-lsprepo, but disable using it for all build actions by default.
I have now implemented 2. here and in swiftlang/sourcekit-lsp#1558, as it's the route I prefer. Let me know what you think.
There was a problem hiding this comment.
I would prefer option (1). Other build scripts (eg. indexstore-db, swift-syntax) also don’t have a clean option.
There was a problem hiding this comment.
I think most have a way to clean the build as part of the build-script and they're usually enabled by default, whether in their build-script-helper script or the bash portion of the overall build-script. I know because I added a lot of the flags to disable them when wanted. 😉
There was a problem hiding this comment.
I still don’t think we should need it but I don’t feel super strongly about it. If you have strong opinions on it being useful, feel free to add it.
But still, is anything ever passing True to indexstoredb.run_build_script_helper?
There was a problem hiding this comment.
I still don’t think we should need it but I don’t feel super strongly about it. If you have strong opinions on it being useful, feel free to add it.
I'd like to have it, in case I need to turn on cleaning for my local builds at some point, as I did recently.
But still, is anything ever passing True to indexstoredb.run_build_script_helper?
No this cleaning flag is set to false in this pull. The idea is just to have the flag available and working, so I can set it to true manually when I need it for my local builds.
If you're okay with that, please review and we can get these two pulls in.
…ave it actually do something Instead, swiftlang/swift#75053 makes sure `build-script` doesn't pass in the flag by default, so you can explicitly invoke it only when needed.
… the flag do nothing Rather than make this change, swiftlang/sourcekit-lsp@37d003eb7 had the `--no-clean` flag do nothing, which means the flag can't be used at all. Instead, switch the flag to `--clean` in swiftlang/sourcekit-lsp#1558 and don't invoke it by default.
|
@swift-ci Please smoke test |
|
@swift-ci Please test |
Rather than make this change, swiftlang/sourcekit-lsp@37d003eb7 had the
--no-cleanflag do nothing, which means the flag can't be used at all.@ahoppen, I'm building sourcekit-lsp locally on my Android phone right now and I'd like to be able to
disenable the--no-clean--cleanflag and have the build cleaned sometimes, so Iwill nexthave submitted a pull to revert the above sourcekit-lsp commit.This pull changes no behavior, since the flag currently does nothing, but let me know if you'd also like me to change the
buildaction here to clean on repeated sourcekit-lsp builds, as it once did.