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

Proper resolution / completion of external workspaces / repositories in label definitions #6664

Open
mtoader opened this issue Aug 26, 2024 · 41 comments
Assignees
Labels
awaiting-maintainer Awaiting review from Bazel team on issues P3 We're not considering working on this, but happy to review a PR. (No assignee) product: IntelliJ IntelliJ plugin type: feature request

Comments

@mtoader
Copy link
Contributor

mtoader commented Aug 26, 2024

Description of the feature request:

As of current master, when looking at a label definition in a BUILD.xxx file (and especially inside bzlmod enabled workspaces) I would like to be able to complete/resolve the visible repository / external workspaces. This doesn't really work as of now

Which category does this issue belong to?

Intellij

What underlying problem are you trying to solve with this feature?

No response

What operating system, Intellij IDE and programming languages are you using? Please provide specific versions.

Current plugin HEAD on mac

Have you found anything relevant by searching the web?

There are some issues related to more general bzl mod support but they seem to have been pushed in the future since internally google didn't adopt this at scale.

Any other information, logs, or outputs that you want to share?

I'm open to work on some of these if acceptable.

@mtoader mtoader added awaiting-maintainer Awaiting review from Bazel team on issues type: feature request labels Aug 26, 2024
@github-actions github-actions bot added the product: IntelliJ IntelliJ plugin label Aug 26, 2024
@mtoader
Copy link
Contributor Author

mtoader commented Aug 26, 2024

To add more context on my thinking here:

I want to call into bazel/blaze mod dump_repo_mapping on sync, store that in a similar way as one of the BlazeProjectData parts and use it inside an "fixed" ExternalWorkspaceFragment reference to provide ergonomic completion and resolution into <executionRoot>/external/workspace name

One open question I have is: should I try to do this using the async mode or the aspect mode?

@mai93
Copy link
Collaborator

mai93 commented Aug 27, 2024

Thanks @mtoader, I think we can accept contributions addressing this issue cc @tpasternak
can you clarify what do you mean by async mode vs aspect mode?

@mai93 mai93 added the P3 We're not considering working on this, but happy to review a PR. (No assignee) label Aug 27, 2024
@mtoader
Copy link
Contributor Author

mtoader commented Aug 27, 2024

/** Interface to persistent project data. */
is kind of basis of a lot of resolving in the current plugin. It has two implementations:

However, the latter is off (the experiment flag is false as the default), as it only seems enabled within Google. It is also incomplete in the current master (see below), which makes me vary about implementing things within it.

@Override
  public TargetMap getTargetMap() {
    throw new NotSupportedWithQuerySyncException("getTargetMap");
  }

  @Override
  public BlazeInfo getBlazeInfo() {
    throw new NotSupportedWithQuerySyncException("getBlazeInfo");
  }
 // ...
  @Override
  public ArtifactLocationDecoder getArtifactLocationDecoder() {
    throw new NotSupportedWithQuerySyncException("getArtifactLocationDecoder");
  }

  @Override
  public RemoteOutputArtifacts getRemoteOutputs() {
    throw new NotSupportedWithQuerySyncException("getRemoteOutputs");
  }

  @Override
  public ExternalWorkspaceData getExternalWorkspaceData() {
    throw new NotSupportedWithQuerySyncException("getExternalWorkspaceData");
  }

  @Override
  public SyncState getSyncState() {
    throw new NotSupportedWithQuerySyncException("getSyncState");
  }

I'm not sure what is the rollout/completion plan of said functionality since if I want to plumb some metadata into BlazeProjectData (as I would have to) it would imply touching this too in some way.

@tpasternak
Copy link
Collaborator

So the qsync module has been introduced some time ago by Google, however at the moment it works well only with Android Studio, as we didn't have capacity to provide the proper support for it. This means, you can skip the qsync part for now

@tpasternak
Copy link
Collaborator

Also, the issue occurs for bzlmod imports. Legacy imports still work well. This comes from the fact that bzlmod stores external repositories in directories, which names are not same as the repositories.

It just fails here, because it merges "external" stem with the repository name, resulting in a directory that doesn't exist

File externalDir = new File(getBlazeProjectData(project).getBlazeInfo().getOutputBase(),
"external/" + workspaceName);
if (externalDir.exists() || isInTestMode()) {
root = new WorkspaceRoot(externalDir);
workspaceRootCache.put(workspaceName, root);
}

@mtoader
Copy link
Contributor Author

mtoader commented Aug 27, 2024

When i checked that didn't do completion at all (neither in classical or bzlmod situatiosn). I recall solving completion when i was within Goog but i don't know what happened with that.

However in order to do it right in bzlmod world we need to use the output of bazel mod dump_repo_mapping workspace (which is what i'm gonna do).

@tpasternak
Copy link
Collaborator

oh, wait, you're right, I meant navigation, not completion

@mtoader
Copy link
Contributor Author

mtoader commented Aug 27, 2024

As of now i have this big change here: master...mtoader:intellij:mtoader/support-module-based-repo-completion. It is still incomplete since I have to fix resolving and tweak the completion UX (fix the tab vs enter vs inside/outside quotes, etc behavior).

My q is: would the reviewers be ok with such a big change or should I make into more manageable pieces?

Note: this is fundamentally big because the ceremony is needed to hook into the sync machinery.

@tpasternak
Copy link
Collaborator

Managable pieces highly appreciated! Thank you for asking btw

mtoader added a commit to mtoader/intellij that referenced this issue Aug 28, 2024
@mtoader
Copy link
Contributor Author

mtoader commented Aug 28, 2024

Posted the first PR in the series

mtoader added a commit to mtoader/intellij that referenced this issue Aug 28, 2024
tpasternak pushed a commit that referenced this issue Aug 28, 2024
* [#6664] Add basic model for ExternalWorkspaces - 1/n

* Update MockProjectDataBuilder

* Fix code style

* Use `Functions.identity()`

* Add rest of the dependencies

* Avoid some unnecessary changes

* Avoid some unnecessary changes
mtoader added a commit to mtoader/intellij that referenced this issue Aug 28, 2024
tpasternak pushed a commit that referenced this issue Aug 29, 2024
* [#6664] Add `blaze mod ...` runner implementation - 2/n

* Revert unnecessary change

* javadocs

* More comments

* Skip using asMap

* Update copyright and use project local output file

* better name + copyright years
@tpasternak
Copy link
Collaborator

Just loose thoughts for later:

  1. we will for sure need a registry flag. I'm almost sure there will be some use case where it just crashes and we will need to provide immediate troubleshooting
  2. Probably we need to handle bazel 5 (still supported)

@mtoader
Copy link
Contributor Author

mtoader commented Aug 29, 2024

So far, I'm making this work only if bazel has mod dump_repo_mapping (aka bazel > 7.1.0). For the others, it will just behave as before.

@tpasternak
Copy link
Collaborator

I know we only have to ensure it doesn't crash

@mtoader
Copy link
Contributor Author

mtoader commented Aug 29, 2024

The plugin? I'm trying to default the ExternalWorkspaceData to EMPTY in all computations. And that should only change if i can infer things from within bazel mod ... (which I gate with bazel version for now).

I could make that execution success optional and warn if it fails. I will see how hard that is.

If your bazel is > 7.1.0 and the blaze mod ... invocation fails, it will break sync. I had to add some workarounds, for example, in PR no: 3, to not have it crash for Clion (where there is a cpp-specific BlazeFlagsProvider that would add a flag unknown to mod).

@tpasternak
Copy link
Collaborator

It would be enough to just check the bazel version and skip the call if it's too low.

@mtoader
Copy link
Contributor Author

mtoader commented Aug 29, 2024

It would be enough to just check the bazel version and skip the call if it's too low.

The last PR actually does that

@mtoader
Copy link
Contributor Author

mtoader commented Sep 4, 2024

workspace works just as any other non-existent repo name: it will result in an error with --noenable_workspace (default in Bazel 8) and return a fallback only relevant with WORKSPACE otherwise. Please test with the flag and you should also see these "implicit" repos go away.

I assumed that running Bazel on demand as an external repo is accessed would be at odds with the plugins sync approach, but if it works well enough, I would recommend that approach. But if that doesn't work out, we can certainly add the recursive lookup to Bazel.

Nice:

➜ bazel mod dump_repo_mapping "" --noenable_workspace  | yq -P
"": ""
com_google_googleapis: googleapis+
buildifier_prebuilt: buildifier_prebuilt+
local_config_platform: local_config_platform
grpc-java: grpc-java+
bazel_tools: bazel_tools
com_google_protobuf: protobuf+
gazelle: gazelle+
rules_go: rules_go+

This works well and will switch to this.

And i will be able to query it's state using bazel info

➜ blaze info --noenable_workspace starlark-semantics
INFO: Invocation ID: c8436437-99d1-48f4-af84-6442e77c9958
StarlarkSemantics{enable_workspace=false, incompatible_use_plus_in_repo_names=true}

Thanks

mtoader added a commit to mtoader/intellij that referenced this issue Sep 5, 2024
  In PR bazelbuild#6711 i added two back to back bazel info calls.

  This PR collapsed  those calls into one.
mtoader added a commit to mtoader/intellij that referenced this issue Sep 5, 2024
mtoader added a commit to mtoader/intellij that referenced this issue Sep 5, 2024
  In PR bazelbuild#6711 i added two back to back bazel info calls.

  This PR collapsed  those calls into one.
@mtoader
Copy link
Contributor Author

mtoader commented Sep 5, 2024

workspace works just as any other non-existent repo name: it will result in an error with --noenable_workspace (default in Bazel 8) and return a fallback only relevant with WORKSPACE otherwise. Please test with the flag and you should also see these "implicit" repos go away.

I assumed that running Bazel on demand as an external repo is accessed would be at odds with the plugins sync approach, but if it works well enough, I would recommend that approach. But if that doesn't work out, we can certainly add the recursive lookup to Bazel.

Running it on demand is making things more complex in a sense (from the IDE / resolving POV), but one doesn't usually edit imported modules (which is the main case for completion). Resolving otoh would need that info but it can be a on demand cost paid once you try to do it first time.

It would be way better if i could take once from a cmd output :)

mtoader added a commit to mtoader/intellij that referenced this issue Sep 5, 2024
  In PR bazelbuild#6711 i added two back to back bazel info calls.

  This PR collapsed  those calls into one.
mtoader added a commit to mtoader/intellij that referenced this issue Sep 5, 2024
  In PR bazelbuild#6711 i added two back to back bazel info calls.

  This PR collapsed  those calls into one.
mtoader added a commit to mtoader/intellij that referenced this issue Sep 5, 2024
mtoader added a commit to mtoader/intellij that referenced this issue Sep 5, 2024
  Also add a CharFilter that enable:
    * '@' to filter the lookup elements and not hide the lookup.
    * '/' to autocomplete selected item

  Other issues: bazelbuild#505
mtoader added a commit to mtoader/intellij that referenced this issue Sep 5, 2024
tpasternak pushed a commit that referenced this issue Sep 5, 2024
Also add a CharFilter that enable:
    * '@' to filter the lookup elements and not hide the lookup.
    * '/' to autocomplete selected item

  Other issues: #505
tpasternak pushed a commit that referenced this issue Sep 5, 2024
In PR #6711 i added two back to back bazel info calls.

  This PR collapsed  those calls into one.
@mtoader
Copy link
Contributor Author

mtoader commented Sep 5, 2024

@tpasternak I have a q. how are changes from this repo "migrated" to https://github.com/JetBrains/hirschgarten ?

@tpasternak
Copy link
Collaborator

Usually they are not, hirschgarten is a full rewrite

@mtoader
Copy link
Contributor Author

mtoader commented Sep 5, 2024

Usually they are not, hirschgarten is a full rewrite

So anything i would want there would have to be redone there? I like clean rewrites :)

@tpasternak
Copy link
Collaborator

Cc @jastice

@tpasternak
Copy link
Collaborator

And cc @ujohnny

@mtoader
Copy link
Contributor Author

mtoader commented Sep 5, 2024

I see no real completion in there as of now (neither respecting bzlmod info nor just basic local repository) so I assume this is not yet there.

@jastice
Copy link
Collaborator

jastice commented Sep 5, 2024

HI @mtoader! Thanks for your contributions to this plugin, they're appreciated. But to give you a bit of context, we are spending over 90% of our efforts on the new plugin in the hirschgarten repo, so mid-term it might be more productive to contribute there. We're happy to work with you to address any feature gaps to help migrate to the new plugin.

@mtoader
Copy link
Contributor Author

mtoader commented Sep 5, 2024

HI @mtoader! Thanks for your contributions to this plugin, they're appreciated. But to give you a bit of context, we are spending over 90% of our efforts on the new plugin in the hirschgarten repo, so mid-term it might be more productive to contribute there. We're happy to work with you to address any feature gaps to help migrate to the new plugin.

For now, I'm happy to complete the work I want here (moving the ideas there would be easier afterward, and I would have to learn Kotlin to be effective).

This was more of a generic question: What is the current progress state? For example, is any completion work being done? Reference searches, etc, yet? (i couldn't tell from the current beta)

@mpiekutowski
Copy link

Hi @mtoader! Currently we have completion/resolution for local references and those from load statements in the main repository. At the moment I am working on completion for labels and keyword arguments. We are also working on adding these features for external workspaces. So yes, completion work is being done right now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-maintainer Awaiting review from Bazel team on issues P3 We're not considering working on this, but happy to review a PR. (No assignee) product: IntelliJ IntelliJ plugin type: feature request
Projects
None yet
Development

No branches or pull requests

11 participants
@mtoader @jastice @tpasternak @fmeum @mpiekutowski @mai93 and others