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

Make quereis to share the same server by default #270

Merged
merged 5 commits into from
Jan 26, 2024

Conversation

limdor
Copy link
Contributor

@limdor limdor commented Jul 25, 2022

The bazel query used to introspect the targets available assumes that the generated files are
already there.
This creates the problem that you have to first generate the files once.
#265

Not sharing the same server makes it harder because the output is a random one.
This means that for a normal user, it will be hard to make it work when query does not
use the same server like your normal bazel queries.
Because of that, it is better to have the queriesShareServer to true by default
and the expert users can set it to false if they want.

The bazel query used to introspect the targets available assumes that the generated files are
already there.
This creates the problem that you have to first generate the files once.
bazel-contrib#265

Not sharing the same server makes it harder because the output is a random one.
This means that for a normal user, it will be hard to make it work when query does not
use the same server like your normal bazel queries.
Because of that, it is better to have the queriesShareServer to true by default
and the expert users can set it to false if they want.
package.json Outdated
@@ -107,7 +107,7 @@
},
"bazel.queriesShareServer": {
"type": "boolean",
"default": false,
"default": true,
"description": "Use the same Bazel server for queries and builds. By default, vscode-bazel uses a separate server for queries so that they can be executed in parallel with builds. You can enable this setting if running multiple Bazel servers has a negative performance impact on your system, but you may experience degraded performance in Visual Studio Code for operations that require queries."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just one note, the description would have to be adapted because it assumes that default is false. Before I do that I would like to know the opinion of the contributors if they would agree on having default to true or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coeuvre @philwo any feedback?

@cpsauer
Copy link

cpsauer commented Feb 26, 2023

+1 on this. Otherwise a bunch of features are broken by default as in @265. Any chance we could get a review?

Thanks,
Chris
(ex-Googler)

@limdor
Copy link
Contributor Author

limdor commented Dec 29, 2023

@jfirebaugh @coeuvre could some of you comment on this PR? Or should I just close it?

@jfirebaugh
Copy link
Collaborator

I'm up for changing this default and seeing how it works. I'm going to put out a 0.8.1 release with a fix for a regression in 0.8.0, and then I'll merge this.

package.json Outdated Show resolved Hide resolved
@limdor
Copy link
Contributor Author

limdor commented Jan 2, 2024

@jfirebaugh knowing that you are opened to the change. I wrote a proposal on how the description could look like to match with the new default.

@cpsauer
Copy link

cpsauer commented Jan 24, 2024

Still down to merge @jfirebaugh--or is this waiting on @limdor?

@limdor
Copy link
Contributor Author

limdor commented Jan 24, 2024

If that is the case let me know because from my side everything is done. In the moment I das informed there is interest to merge it I also update the message to fit to the new default.

@cameron-martin
Copy link
Collaborator

cameron-martin commented Jan 24, 2024

The bazel query used to introspect the targets available assumes that the generated files are
already there.
This creates the problem that you have to first generate the files once.

I think this diagnosis is wrong. The issue in #265 looks to be caused by bazel traversing the convenience symlinks when gathering packages (see bazelbuild/bazel#10653). I've opened up a PR in Bazel to mitigate this but I think enabling this option by default is a good option in the meantime.

Copy link
Collaborator

@cameron-martin cameron-martin left a comment

Choose a reason for hiding this comment

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

Until bazelbuild/bazel#10653 is fixed, I think this is a reasonable workaround.

@cameron-martin cameron-martin merged commit 64c9d7e into bazel-contrib:master Jan 26, 2024
4 checks passed
@cpsauer
Copy link

cpsauer commented Jan 27, 2024

Yay! Thanks, Cameron. I think that'll help a lot of new folks get benefit out of the box--once released.

cameron-martin added a commit that referenced this pull request Mar 12, 2024
This reverts commit 64c9d7e.

The underlying issue that this addresses is now fixed in Bazel 7.1.0. See #216 and bazelbuild/bazel#21505.
cameron-martin added a commit to cameron-martin/vscode-bazel that referenced this pull request Mar 13, 2024
)"

This reverts commit 64c9d7e.

The underlying issue that this addresses is now fixed in Bazel 7.1.0. See bazel-contrib#216 and bazelbuild/bazel#21505.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants