Skip to content

[build] Disable cleaning sourcekit-lsp by default, rather than having the flag do nothing #75053

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 23, 2024
Merged

[build] Disable cleaning sourcekit-lsp by default, rather than having the flag do nothing #75053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions utils/swift_build_support/swift_build_support/products/indexstoredb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,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):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was asking a different question, I think. I see two options here:

  1. Remove all code related to cleaning sourcekit-lsp builds.
  2. Bring back the cleaning code in the sourcekit-lsp repo, 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.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer option (1). Other build scripts (eg. indexstore-db, swift-syntax) also don’t have a clean option.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

script_path = os.path.join(
product.source_dir, 'Utilities', 'build-script-helper.py')

Expand Down Expand Up @@ -113,8 +113,8 @@ def run_build_script_helper(action, host_target, product, args,
elif args.enable_tsan:
helper_cmd.extend(['--sanitize', 'thread'])

if not clean:
helper_cmd.append('--no-clean')
if clean:
helper_cmd.append('--clean')

# Pass Cross compile host info unless we're testing.
# It doesn't make sense to run tests of the cross compile host.
Expand Down
4 changes: 2 additions & 2 deletions utils/swift_build_support/swift_build_support/products/sourcekitlsp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@ def should_test(self, host_target):
def test(self, host_target):
indexstoredb.run_build_script_helper(
'test', host_target, self, self.args,
self.args.test_sourcekitlsp_sanitize_all, clean=False)
self.args.test_sourcekitlsp_sanitize_all)

def should_install(self, host_target):
return self.args.install_sourcekitlsp

def install(self, host_target):
indexstoredb.run_build_script_helper(
'install', host_target, self, self.args, clean=False)
'install', host_target, self, self.args)

@classmethod
def get_dependencies(cls):
Expand Down