Followup for Pass path associations as special compiler flags #6007
Conversation
LeFrosch
requested review from
mai93,
jastice,
tpasternak and
agluszak
as code owners
github-actions
bot
added
product: CLion
|
I remember when I intentionally added a check in VirtualIncludesHandler.java to handle this. Something should be definitely done to it, either removed or message there should be updated. |
| } | ||
| } | ||
|
|
||
| private List<String> collectIncludes(Path root, TargetKey targetKey, BlazeProjectData blazeProjectData) { |
There was a problem hiding this comment.
I'd pass indicator to this routine as well and performed a check indicator.checkCanceled() in both for loops.
| try { | ||
| return Path.of(value); | ||
| } catch (InvalidPathException e) { | ||
| return null; |
There was a problem hiding this comment.
It should be logged, but please warn.
There was a problem hiding this comment.
some indentations are wrong in this file in if blocks, please make sure you have style guide applied
| @@ -257,6 +355,9 @@ private OCWorkspaceImpl.ModifiableModel calculateConfigurations( | |||
| .map(file -> "-I" + file.getAbsolutePath()) | |||
| .collect(toImmutableList()); | |||
|
|
|||
| Path rootPath = workspaceRoot.directory().toPath(); | |||
| List<String> includes = collectIncludesWithProgress(rootPath, targetKey, blazeProjectData, indicator); | |||
There was a problem hiding this comment.
these includes are quite specific ones. should we say something like collectBazelStripIncludeHints?
| continue; | ||
| } | ||
|
|
||
| // if absolut strip prefix is a repository-relative path |
|
|
||
| CIdeInfo cIdeInfo = targetIdeInfo.getcIdeInfo(); | ||
| Path includePrefix = pathOf(cIdeInfo.getIncludePrefix()); | ||
| Path stripPrefix = pathOf(cIdeInfo.getStripIncludePrefix()); |
There was a problem hiding this comment.
related to #5969, strip prefix might be //foo/bar and this code will break on Windows (yes we do not support it, though it works and maybe we should not introduce new issues).
Changes: - formatting and names - only collects hints for include_prefix, strip_include_prefix is handled by VirtualIncludesHandler - no longer recursive and handles duplicated dependencies
- 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
…xternal workspaces
This should be redundant because includes are mapped with the -ibazel flag
LeFrosch
force-pushed
the
Daniel.Brauner/5301
branch
from
190f4cc
to
4b74423
Compare
| @@ -107,7 +107,7 @@ public String externalWorkspaceName() { | |||
| } | |||
| int slashesIndex = label.indexOf("//"); | |||
| logger.assertTrue(slashesIndex >= 0); | |||
| return label.substring(1, slashesIndex); | |||
| return label.substring(0, slashesIndex).replaceFirst("^@+", ""); | |||
There was a problem hiding this comment.
I believe there's something wrong here, this line probably should not be changed since in bazel external workspace name should has only one @ symbol
There was a problem hiding this comment.
I see that it's the way bazel 7 started to report external workspace targets. Investigating.
There was a problem hiding this comment.
So my conclusion here is the following:
- Please drop this change from this patchset.
- Create new issue describing issue about bazel 7 -- this code works fine with bazel 6.
- Please check any other occurrences of
@strip and adjust them. - Should we introduce some special function that expects exactly 1 or exactly 2
@signs?
There was a problem hiding this comment.
Left a comment in this thread
https://github.com/bazelbuild/intellij/pull/6007/files#r1514537317
github-actions
bot
removed
the
awaiting-review
Checklist
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
Followup for #5301. Pass all headers in targets with
include_prefixwith special "-ibazel" flag like that they can be handled by CLion. Also I tried to address all open comments in the pull request.