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

Trying to workaround "unsynced" case for newly added files. #5341

Conversation

dkashyn-sfdc
Copy link
Contributor

@dkashyn-sfdc dkashyn-sfdc commented Sep 13, 2023

Checklist

  • I have filed an issue about this change and discussed potential changes with the maintainers.
  • I have received the approval from the maintainers to make this change.
  • This is not a stylistic, refactoring, or cleanup change.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.

Discussion thread for this change

Issue number: <please reference the issue number or url here>

Description of this change

#5347

@github-actions github-actions bot added product: CLion CLion plugin product: IntelliJ IntelliJ plugin product: GoLand GoLand plugin awaiting-review Awaiting review from Bazel team on PRs labels Sep 13, 2023
@mai93
Copy link
Collaborator

mai93 commented Sep 18, 2023

This can be fixed by running "Partial Sync" on the newly added file, can you explain why this fix is needed?

@dkashyn-sfdc
Copy link
Contributor Author

Because when you refactoring/adding files, unsynced by default is confusing.

Of course Partial Sync can help but:

  • it takes 1 minute for our project if you are lucky
  • with existing IJ project structure this operation is excessive so we can use sync status of surrounding files
  • it is so opaque and unapparent to our users that manual sync is required for every move/add operation

@mai93 mai93 assigned blorente and unassigned mai93 Sep 25, 2023
@blorente
Copy link
Collaborator

Sorry for the late review. The more I think about it, the more I am against this sort of change, because it will expose lies to users.

I agree that we should have a lightweight way of updating the SourceToTargetMap for just one target that essentially expands the globs in srcs. Does Bazel -> Sync -> Sync Directories work for this case? It should be much faster, since it doesn't build anything with Bazel.

@dkashyn-sfdc
Copy link
Contributor Author

@blorente, sorry for being a little bit blunt here, but we are having issues even with the first "Bazel Sync" need, with our engineers. This operation is so unnatural for IJ.

"Sync" after any refactor or file changes might be okay, for Google with your huge project and associated tooling, but for mere mortals outside of Google, this is so unnatural that people are really struggling with this remedy.

Even more: a few of hundreds of our engs are fluent in all 5 types of syncs that Bazel Plugin has.

@ujohnny
Copy link
Collaborator

ujohnny commented Oct 18, 2023

Folks, let me jump in here. It looks like this change might lead to an incorrectly analyzed code, for example, in CLion as the target-level settings won't be applied, and a file will be analyzed with project-wide settings. I agree that any sync is a too rough solution when a single file is added/moved, but there's a lot of code that should be run for every file in the project model, at least for CLion to get correct code analysis, and so far that code can be reached only from sync routines. If someone can add a listener for a new file / moved file and "implement a part of bazel" (ie, detect target to which it did belong, which it will belong) on the IntelliJ side and update the workspace state properly, then it will be really cool. Otherwise, I am afraid of new reported issues regarding "red code," etc.

@dkashyn-sfdc
Copy link
Contributor Author

dkashyn-sfdc commented Oct 18, 2023

Well, this was never 100%-complete solution since if the package is empty there is nothing to triangulate against.

Also if we are talking about tests there won't be consensus in targets (packages?) for those.

@dkashyn-sfdc
Copy link
Contributor Author

We decided not to proceed with this PR and to keep it to our internal version only :( I wish BSP were here faster!

@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin
Projects
Development

Successfully merging this pull request may close these issues.

4 participants