Pass path associations as special compiler flags #5301

yvvan · 2023-09-01T10:54:14Z

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: #5300

Description of this change

Pass all headers in targets with include_prefix with special "-ibazel" flag like so that they could be handled by CLion.

google-cla · 2023-09-01T10:54:18Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

sgowroji · 2023-09-01T11:20:13Z

Hi @yvvan, Could you please sign the CLA?

yvvan · 2023-09-01T11:29:01Z

Hi @yvvan, Could you please sign the CLA?

I was added to our JetBrains list. My commit also has my JetBrains email.

tpasternak

So when I tried to use this code on the example below, and try to jump to lib_header.h from app.cpp, I still got "This file does not belong to any project target" error.

The other comments are stylistic, so feel free to ignore them.

     File: app/app.cpp
   1 #include "lib/lib_header.h"
   2 
   3 int main () {
   4     f();
   5 }

     File: app/BUILD
   1 cc_binary(
   2     name = "app",
   3     srcs = ["app.cpp"],
   4     deps = ["//lib:lib"],
   5 )

     File: lib/BUILD
   1 cc_library(
   2     name = "lib",
   3     srcs = ["lib.cpp"],
   4     hdrs = ["include/lib_header.h"],
   5     strip_include_prefix = "include",
   6     include_prefix = "lib",
   7     visibility = ["//visibility:public"],
   8 )

     File: lib/include/lib_header.h
   1 #pragma once
   2 
   3 int f();

     File: lib/lib.cpp
   1 #include "lib/lib_header.h"
   2 
   3 int f() {
   4     return 0;
   5 }

tpasternak · 2023-09-05T14:27:48Z

cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java

@@ -22,8 +22,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.errorprone.annotations.Keep;
-import com.google.idea.blaze.base.ideinfo.TargetIdeInfo;
-import com.google.idea.blaze.base.ideinfo.TargetKey;
+import com.google.idea.blaze.base.ideinfo.*;


iirc we try to avoid asterisk imports cc @mai93

That's right, we prefer specific imports for the classes you need.

tpasternak · 2023-09-05T14:27:51Z

cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java

        // defines and include directories are the same for all sources in a given target, so lets
        // collect them once and reuse for each source file's options

        UnfilteredCompilerOptions coptsExtractor =
            UnfilteredCompilerOptions.builder()
                .registerSingleOrSplitOption("-I")
-                .build(targetIdeInfo.getcIdeInfo().getLocalCopts());
+                .build(cIdeInfo.getLocalCopts());


We still have two release branches - google and master. We regularly cherry pick commits from google to master. For this reason we try to keep the changes as small as possible. If it's refactoring-only, I'd prefer to keep the targetIdeInfo.getcIdeInfo calls

@tpasternak The changes are not supported by any public CLion version. It will be in the closest CLion 2023.3 EAP or in the next 2023.2.x release.

@tpasternak Ok, I will remove my small refactoring which is not really necessary.

- take library path into account for path transformation - don't use imports with `*` - revert small refactoring

tpasternak · 2023-09-22T12:29:59Z

cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java

+        }
+        if (includePrefix != null && !includePrefix.isEmpty()) {
+          pathUsedInSourceCode = includePrefix + "/" + pathUsedInSourceCode;
+          System.out.println("updated path 2 = " + pathUsedInSourceCode);


Please remove these print statements or change to logger calls

cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java

tpasternak · 2023-09-22T12:39:37Z

cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java

+      String realPath = rootPath + "/" + header.getExecutionRootRelativePath();
+      String libPath = targetKey.getLabel().blazePackage().asPath().toString();
+      String pathUsedInSourceCode = header.getRelativePath();


How about using Path instead of String in many places here? It could be much safer imo, particularly when we call .startsWith().

For example "libA/header.h".startsWith("lib") would return true for Strings, but false for Paths.

We also wouldn't have to call things like (stripPrefix.endsWith("/") ? 0 : 1)

tpasternak · 2023-09-22T12:42:27Z

cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java

@@ -247,6 +292,9 @@ private OCWorkspaceImpl.ModifiableModel calculateConfigurations(
                .map(file -> "-I" + file.getAbsolutePath())
                .collect(toImmutableList());

+        String rootPath = workspaceRoot.directory().getAbsolutePath();
+        ImmutableList<String> includes = ImmutableList.copyOf(collectIncludes(rootPath, targetKey, blazeProjectData));


IMO collectIncludes method might be quite heavy, because it traverses whole dependency graph. I'd prefer to

Add a registry key that allows users to disable it at all in case it's too heavy for them and they don't use include_prefix

Measure time and print it to the user or (maybe even better) to run it with Progress Indicator so the users could observe that this particular step significantly impacts them

Would you like to take over this pull request? I'm not working on bazel currently and have less knowledge on the plugin source code.
Also I don't think this step is long - it may take time only if enough entries are specified so if one doesn't use this feature at all than it wouldn't cost time. But if you are still worried we may make a progress indicator here.

- take library path into account for path transformation - don't use imports with `*` - revert small refactoring

- added registry key - added progress indicator - using path instead of string transformations

- take library path into account for path transformation - don't use imports with `*` - revert small refactoring

- added registry key - added progress indicator - using path instead of string transformations

Pass path associations as special compiler flags

823b31b

yvvan requested review from mai93, jastice and tpasternak as code owners September 1, 2023 10:54

github-actions bot added product: CLion CLion plugin awaiting-review Awaiting review from Bazel team on PRs labels Sep 1, 2023

sgowroji added awaiting-user-response Awaiting response from author on PRs and removed awaiting-review Awaiting review from Bazel team on PRs labels Sep 1, 2023

sgowroji added awaiting-review Awaiting review from Bazel team on PRs and removed awaiting-user-response Awaiting response from author on PRs labels Sep 1, 2023

sgowroji assigned tpasternak Sep 4, 2023

tpasternak suggested changes Sep 5, 2023

View reviewed changes

Address review comments in bazelbuild#5301 pull request

631db24

- take library path into account for path transformation - don't use imports with `*` - revert small refactoring

tpasternak reviewed Sep 22, 2023

View reviewed changes

tpasternak suggested changes Sep 22, 2023

View reviewed changes

LeFrosch pushed a commit to LeFrosch/intellij-bazel that referenced this pull request Jan 29, 2024

Address review comments in bazelbuild#5301 pull request

4694229

- take library path into account for path transformation - don't use imports with `*` - revert small refactoring

LeFrosch added a commit to LeFrosch/intellij-bazel that referenced this pull request Feb 1, 2024

Address review comments in bazelbuild#5301 pull request

96be0a3

- added registry key - added progress indicator - using path instead of string transformations

LeFrosch mentioned this pull request Feb 1, 2024

Followup for Pass path associations as special compiler flags #6007

Merged

3 tasks

LeFrosch pushed a commit to LeFrosch/intellij-bazel that referenced this pull request Feb 19, 2024

Address review comments in bazelbuild#5301 pull request

9f90f33

- take library path into account for path transformation - don't use imports with `*` - revert small refactoring

LeFrosch added a commit to LeFrosch/intellij-bazel that referenced this pull request Feb 19, 2024

Address review comments in bazelbuild#5301 pull request

d58d4ca

- added registry key - added progress indicator - using path instead of string transformations

ujohnny closed this Mar 7, 2024

github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass path associations as special compiler flags #5301

Pass path associations as special compiler flags #5301

yvvan commented Sep 1, 2023 •

edited

Loading

google-cla bot commented Sep 1, 2023

sgowroji commented Sep 1, 2023

yvvan commented Sep 1, 2023 •

edited

Loading

tpasternak left a comment •

edited

Loading

tpasternak Sep 5, 2023

mai93 Sep 5, 2023

tpasternak Sep 5, 2023

yvvan Sep 6, 2023 •

edited

Loading

yvvan Sep 6, 2023

tpasternak Sep 22, 2023

tpasternak Sep 22, 2023

tpasternak Sep 22, 2023

yvvan Nov 20, 2023

Pass path associations as special compiler flags #5301

Pass path associations as special compiler flags #5301

Conversation

yvvan commented Sep 1, 2023 • edited Loading

Checklist

Discussion thread for this change

Description of this change

google-cla bot commented Sep 1, 2023

sgowroji commented Sep 1, 2023

yvvan commented Sep 1, 2023 • edited Loading

tpasternak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yvvan Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yvvan commented Sep 1, 2023 •

edited

Loading

yvvan commented Sep 1, 2023 •

edited

Loading

tpasternak left a comment •

edited

Loading

yvvan Sep 6, 2023 •

edited

Loading