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 //clwb(plugin) and //cpp(plugin) dependency on cidr.lang optional: plugin xmls split #5473

Merged
merged 5 commits into from
Oct 18, 2023

Conversation

ujohnny
Copy link
Collaborator

@ujohnny ujohnny commented Oct 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: #5472

Description of this change

Long story short: com.intellij.modules.cidr.lang is going to become optional in CLion at some point. All the usages of code exported from com.intellij.modules.cidr.lang are moved to optional plugin xmls, so plugin won't emit lots of exceptions and break the IDE if the mentioned module is not available.

A little bit more detailed here. We (CLion team) are working on deprecation of the old language engine. Parts of it which are not going to be deprecated are moved to cidr-base-plugin library, which is a part of CIDR Base (com.intellij.cidr.base) essential CLion plugin, so any symbol from cidr-base-plugin will always be available. Though that does not apply to c-plugin library which is a part of com.intellij.modules.cidr.lang module.

At this point the goal of this change is to make Bazel for CLion tolerant to disabled com.intellij.modules.cidr.lang. We're going to contribute further to adjust existing code which is moved to *.oclang(.*)? packages in the review to preserve the features when only the new language engine is available.

Copy link
Collaborator

@blorente blorente left a comment

Choose a reason for hiding this comment

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

LGTM. Tested it on CLion 2023.2 with no issues. Just one small comment, then I think it's good on my end.

VirtualFile getSourceFileForHeaderFile(Project project, VirtualFile headerFile);

static VirtualFile findAndGetSourceFileForHeaderFile(Project project, VirtualFile headerFile) {
SourceFileFinder finder = EP_NAME.getPoint().extensions().findFirst().orElse(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make it hard-error if there are more than one registered EPs here? Otherwise there's nothing telling us we shouldn't register more than one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for checking the review. I updated this part and similar part of the code in clwb.

@ujohnny ujohnny force-pushed the enovozhilov/optional-cidr-lang branch from 0784fbf to 5d74726 Compare October 18, 2023 08:52
@jastice jastice merged commit a00e775 into bazelbuild:master Oct 18, 2023
6 checks passed
@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Oct 18, 2023
@jastice jastice self-assigned this 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
Projects
Development

Successfully merging this pull request may close these issues.

4 participants