Skip to content

Conversation

@zjturner
Copy link
Contributor

@zjturner zjturner commented Sep 3, 2025

vsgo by default uses the actual include path that it passes to the compiler for intellisense purposes. This is obviously correct from a compilation point of view, but it creates serious issues with intellisense usability.

  1. When a header is opened from buck-out, it's a symlink to the source tree. But Visual Studio doesn't look through the symlink, so it thinks it's a completely different file from the one in the project. This leads to frequently having 2 copies of the same file opened in an editor.
  2. When a header is opened from buck-out, since it's not part of any project (again, VS doesn't look through the symlink) Visual Studio does not know what settings to use for intellisense. This means that if you right click an include statement from a cpp file and open the header file that way, you will get a header file in buck-out, and then since VS doesn't know what settings to use for Intellisense, the whole file will be filled with squigglies.

This PR addresses both of these issues by forcing raw headers mode whenever possible. This is only for Intellisense purposes, nothing about the behavior of executing a build is changed. It does this by copying the logic of headers_as_raw_headers from the cxx prelude. Unfortunately I was not able to reuse that function directly out of the prelude since a BxlContext and an AnalysisContext are not compatible types.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 3, 2025
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D81630869. (Because this pull request was imported automatically, there will not be any future comments.)

@autozimu
Copy link
Contributor

autozimu commented Sep 4, 2025

This is relatively large code changes. I will need one or two days to understand it a little bit more and ensure there is no regression on our internal (massive) projects.

@zjturner
Copy link
Contributor Author

zjturner commented Sep 4, 2025

No worries, thank you! Here's the first test I would do to see the difference in practice (using vsgo without this PR):

  1. Generate a solution
  2. Open a cpp file from the solution explorer
  3. Go to a statement like #include "Foo.h". Right click it and choose Go to header file
  4. Find that same header file in solution explorer and double click it.

Actual behavior (without this PR): Two copies of the file will be open in the editor. One with a symlink path and one with source tree path. The version with symlink path will be full of squigglies and intellisense won't work. The other version everything will work.
Expected behavior (with this PR): There should only be 1 copy of the header file open in the editor, and it should not have any squigglies.

Second test you can do is as follows:

  1. Generate a solution
  2. Right click a project in solution explorer and go to Properties -> C/C++ -> Include directories.
  3. Expand the "Additional Include Directories" field

Previous behavior: Lots of paths to buck-out.
New behavior: Most buck-out paths are replaced with paths into the source tree.

@autozimu
Copy link
Contributor

autozimu commented Sep 4, 2025

CC @zhaopuw

@autozimu
Copy link
Contributor

autozimu commented Sep 4, 2025

I generated some solutions for both before and after this change, and I think I found some issue.

  1. Extra target project root is included. See the example in https://gist.github.com/autozimu/59c68297086292acb5905b358e1d9a54 where $(RepoRoot)\third-party\semver is an extra path and it is target project root.
  2. Probably same cause, but I see very high level directories get included. See example in https://gist.github.com/autozimu/eca8df0385e9208b03645a7becc41661 where $(RepoRoot)\xplat\folly\.. and $(RepoRoot)\xplat\folly\portability\..\... There are two sub-problems. 1) they ideally shall resolved and deduplicated to $(RepoRoot)\xplat 2) $(RepoRoot)\xplat probably shouldn't be there.

(GitHub UI isn't very efficient to spot the difference, might want to other tools for viewing the diff.)

Our internal solutions easily pass thousands of projects, with the fact that some of our folder is enormous like "$(RepoRoot)\xplat". If all target root or large directory is included, it stalls Visual Studio completely as Visual Studio needs to enumerate and index all the files in include path. We got bite by the same problem here last year and got around by rigorously examine what included in include paths.

@zjturner
Copy link
Contributor Author

zjturner commented Sep 4, 2025

If I’m reading the diff correctly, I’m guessing that somewhere in your codebase, there is the statement #include “include/semver.hpp” and the actual file path on disk is third-party\\semver\\include\\semver.hpp.

if this is correct then i think the new algorithm is actually correct. On the other hand, it obviously is a non starter for visual studio project to fail to load, so we have to find some solution. Possible ideas (just brainstorming)

  1. You could change the code to write #include “semver.hpp” so that semver folder doesn’t need to be on include path.
  2. You could use header namespace to disable raw header translation for that file / project
  3. We could add a flag to the bxl to enable raw headers, and fall back to old algorithm if it’s off.

that being said, i think everyone will benefit from raw headers mode here, because without it — at least for our use cases — Intellisense is totally broken on headers most of the time. So I’d like to find a way to make it the default, but I’m also open to doing whatever change causes the least disruption for you

@autozimu
Copy link
Contributor

autozimu commented Sep 4, 2025

Here is the include part for the first example,

#include <semver.hpp>

We just buckified https://github.com/Neargye/semver internally with,

fb_native.cxx_library(
    name = "semver",
    exported_headers = ["include/semver.hpp"],
    public_include_directories = ["include"],
)

fb_native.cxx_binary(
    name = "basic_example",
    srcs = ["example/basic_example.cpp"],
    deps = [":semver"],
)

I then generated solution for target basic_example.

If I understand correctly, this should be the exported public include path for semver library (and thus included when other projects depends on it such as basic_example),

third_party/semver/include

While the currently generated include path has an extra third-party/semver, 1) which is simply wrong as it's not declared as public header. Even though IntelliSense is perfectly working, when including new headers, it can be in a situation that IDE and buck build doesn't agree with each other. 2) can significantly slows down VS indexing.

I'm all for solving the problems for both internally and externally. We might need to address the rough edges or unintended behavior of this code. In the worst case, we could branch out and do things differently (might caused by macro layer differences etc).

@zjturner
Copy link
Contributor Author

zjturner commented Sep 4, 2025

Ok i think i understand. You are using public_include_directories, so in your case Intellisense will already find includes includes in the source tree.

we are not using public_include_directories because it causes local builds to break the sandbox and be different than remote builds. 🤔

@zjturner
Copy link
Contributor Author

zjturner commented Sep 4, 2025

Can you confirm that when you right click an include statement in the editor to open the header that way, does the opened file have a symlink path to buck-out, or source tree path?

@autozimu
Copy link
Contributor

autozimu commented Sep 5, 2025 via email

@zjturner
Copy link
Contributor Author

zjturner commented Sep 5, 2025

For this example, I see opened file from the source tree, no buck-out in the path. If not public_include_header, what is use in your repo? header namespace? Only one way? Meta's codebase is massive, that example is just one I use for quick validation. Our repo is very much likely have setup in all the different ways.

We use headers and exported_headers, but it’s not necessary to use public_include_directories or include_directories when using headers / exported_headers

@zjturner
Copy link
Contributor Author

zjturner commented Sep 5, 2025

What if we were to say that it only uses this method if there is an empty public_include_directories attribute and a non-empty exported_headers attribute?

@autozimu
Copy link
Contributor

autozimu commented Sep 5, 2025

I see. I believe if public_include_directories is empty, it should behave the same way that it's not used.

Can you also do the exercise of generating solutions before and after this PR and diff them?

I understand this debugging process is not easy. I can help making this works for both public_include_directories and exported_headers, and verify on our internal projects, though I might have limited time here for other priorities.

@zjturner
Copy link
Contributor Author

zjturner commented Sep 5, 2025

Just to make sure we're talking about the same thing, what I mean is that suppose I make a change like follows: If exported_headers and public_include_directories are both specified and non-empty, then the behavior will be unchanged from the way it works today (without this PR). This means your builds should (at least in theory) be unaffected. For targets where exported_headers exists, but public_include_directories is either empty or unspecified, it will use the new approach in this PR.

Does this sound like a reasonable approach?

@zjturner
Copy link
Contributor Author

zjturner commented Sep 5, 2025

FWIW, the reason we don't use public_include_directories is because for remote actions, it doesn't have any effect. And for local actions it means the compiler will search the local source tree, which is non-hermetic. So your local and remote builds have different semantics.

@autozimu
Copy link
Contributor

autozimu commented Sep 5, 2025

Sounds good.

When I say, Meta's targets are setup using both exported_headers + public_include_directories, I meant they're used on different targets. For individual target, only one of them is non-empty. And the far majority I saw in our internally repo are using public_include_directories.

@zjturner
Copy link
Contributor Author

zjturner commented Sep 5, 2025

That’s interesting, I actually find that surprising since I thought that’s the “wrong” way. Can you find an internal target using exported_headers and see if Intellisense works for it?

@autozimu
Copy link
Contributor

autozimu commented Sep 5, 2025

Build infra people might know this better, and we can also bring the question to them. But based on the doc https://buck2.build/docs/prelude/rules/cxx/cxx_library/ and extensive usage of public_include_directories internally, I don't think it's considered deprecated or less supported.

Checking the case of exported_headers from our internal repo.

@autozimu
Copy link
Contributor

autozimu commented Sep 5, 2025

I've checked prod example of exported_headers in our internal repo, and jumping to header works fine (without this PR), and I don't see unnecessary buck-out in the AdditionalIncludeDirectories.AdditionalIncludeDirectories

The library definition,

...
    exported_headers = subdir_glob(
        [
            ("src/include/C2PA", "*.hpp"),
            ("include/C2PA", "*.hpp"),
        ],
        prefix = "C2PA",
    ),
...

The AdditionalIncludeDirectories of the generated reverse dependency target's vcxproj,

...
$(RepoRoot)\third-party\c2pa;$(RepoRoot)\third-party\c2pa\src\include;$(RepoRoot)\third-party\c2pa\include
...

@autozimu
Copy link
Contributor

autozimu commented Sep 5, 2025

If I look closer to that library's intermediate output c2pa.attrs.json, it's clear that its exported headers are within the source tree, not in buck-out,

...
  "exported_headers": {
    "C2PA/AIValidationReportGenerator.hpp": "third-party\\c2pa\\src\\include\\C2PA\\AIValidationReportGenerator.hpp",
    "C2PA/AssetDataProvider.hpp": "third-party\\c2pa\\src\\include\\C2PA\\AssetDataProvider.hpp",
    "C2PA/BMFFMediaSpecificHelper.hpp": "third-party\\c2pa\\src\\include\\C2PA\\BMFFMediaSpecificHelper.hpp",
    "C2PA/BaseBoxesFileHasher.hpp": "third-party\\c2pa\\src\\include\\C2PA\\BaseBoxesFileHasher.hpp",
    "C2PA/C2PAConfig.hpp": "third-party\\c2pa\\src\\include\\C2PA\\C2PAConfig.hpp",
    "C2PA/C2PAFactory.hpp": "third-party\\c2pa\\include\\C2PA\\C2PAFactory.hpp",
....

Maybe there is mode file difference? Or the original examples you're looking at are generated headers during build?

@zhaopuw
Copy link

zhaopuw commented Sep 5, 2025

@zjturner I think it's a little hard to debug here since we are using different buck modes. Can you share the buck mode setting on these configs?

--config=cxx_#default.raw_headers_as_headers_mode=enabled
--config=cxx_#default.private_headers_symlinks_enabled=False
--config=cxx_#default.public_headers_symlinks_enabled=False

If your mode doesn't have them can you add these lines to the mode file and see if the original issue persist?

@zjturner
Copy link
Contributor Author

zjturner commented Sep 6, 2025

If I look closer to that library's intermediate output c2pa.attrs.json, it's clear that its exported headers are within the source tree, not in buck-out,

When you use exported_headers, it will create a folder in buck-out, and then it will create symlinks in buck-out to each file in the header-map. In your example, you would end up with a folder in buck-out like this:

buck-out
|__ ...
     |__ buck-headers
          |__ C2PA
                |__ AIValidationReportGenerator.hpp   --> third-party\\c2pa\\src\\include\\C2PA\\AIValidationReportGenerator.hpp
                |__ AssetDataProvider.hpp   --> third-party\\c2pa\\src\\include\\C2PA\\AssetDataProvider.hpp
                |__ BMFFMediaSpecificHelper.hpp   --> third-party\\c2pa\\src\\include\\C2PA\\BMFFMediaSpecificHelper.hpp
                |__ BaseBoxesFileHasher.hpp   --> third-party\\c2pa\\src\\include\\C2PA\\BaseBoxesFileHasher.hpp
                |__ C2PAConfig.hpp   --> third-party\\c2pa\\src\\include\\C2PA\\C2PAConfig.hpp
                |__ C2PAFactory.hpp   --> third-party\\c2pa\\include\\C2PA\\C2PAFactory.hpp

Then, when generating the argsfile, it will contain something like -Ibuck-out/.../buck-headers. Now, when you have some code like #include <C2PA/AIValidationReportGenerator.hpp> the compiler will locate the file in buck-out.

If you use public_include_directories instead, then you wont' have this -Ibuck-out/.../buck-headers in your argsfile, you will instead just have -Ithird-party/c2pa/src/include.

In the first case (our case), right clicking header file from an open editor in Visual Studio and opening the header file, Intellisense will locate it at buck-out/.../buck-headers, because that's what the -I on the command line said.

In the second case (your case), it will locate it directly in the source tree.

I think looking at c2pa.attrs.json is a little misleading, because what really matters is what directories are passed to the compiler via -I, as that's where it will search for header files to open. For us, when we have a target like this:

cxx_library(
    name = "foo", 
    exported_headers = {
        "foo.h": "foo.h"
    }

Then there is no -I on the command line pointing into the source tree. Only one pointing into buck-out. So that's the root of the problem. In your case, like this:

cxx_library(
    name = "foo", 
    public_include_directories=["."],
    exported_headers = {
        "foo.h": "foo.h"
    }

now the compiler gets a -I that points into the source tree, so it works.

@zjturner I think it's a little hard to debug here since we are using different buck modes. Can you share the buck mode setting on these configs?

--config=cxx_#default.raw_headers_as_headers_mode=enabled
--config=cxx_#default.private_headers_symlinks_enabled=False
--config=cxx_#default.public_headers_symlinks_enabled=False
If your mode doesn't have them can you add these lines to the mode file and see if the original issue persist?

I have never heard of these. I'll look into it. What is this # syntax? I've never seen it before. This is something that is already built into the upstream prelude and not behind fbcode or something?

@autozimu
Copy link
Contributor

autozimu commented Sep 6, 2025

In the first case (our case), right clicking header file from an open editor in Visual Studio and opening the header file, Intellisense will locate it at buck-out/.../buck-headers, because that's what the -I on the command line said.

In the second case (your case), it will locate it directly in the source tree.

This is the part I am trying to figure out here.

I can confirm for our case, argsfiles indeed passes -Ibuck-out\\v2\\gen\\fbsource\\third-party\\c2pa\\__c2pa__\\b0347f5db2eefa79\\buck-headers to the compiler, which is the same case as yours.

But the during VSGO generation, we're not relying on the compiler args (maybe we will eventually), and we're using this heuristic to "guess" the correct include path from the target (before and after this PR).

While this heuristic guess works for us (for both public_include_directories and exported_headers cases), but not for your case (exported_headers). That's the part we don't understand yet, and the c2pa.attrs.json is one step to pinpoint where that difference might come from. Maybe you can share output of your buck2 cquery -A @mode target and *.attrs.json file. That would help a lot here.

@autozimu
Copy link
Contributor

autozimu commented Sep 6, 2025

Alternatively, it might work better to have a repo setup to reproduce the original issue (or future issue) that both can access.

@zjturner
Copy link
Contributor Author

zjturner commented Sep 6, 2025

I can confirm for our case, argsfiles indeed passes -Ibuck-out\v2\gen\fbsource\third-party\c2pa\c2pa\b0347f5db2eefa79\buck-headers to the compiler, which is the same case as yours.

But the during VSGO generation, we're not relying on the compiler args (maybe we will eventually), and we're using this heuristic to "guess" the correct include path from the target (before and after this PR).

Do you also have public_include_directories set on that target? If so both paths will be on the command line in the argsfile.

I think I know which heuristic you’re referring to. Maybe one difference is that we always (100% of the time) use a header_namespace (usually “.”)

@zhaopuw
Copy link

zhaopuw commented Sep 8, 2025

What is this # syntax? I've never seen it before. This is something that is already built into the upstream prelude and not behind fbcode or something?

That's the cxx_#default toolchain type that vsgo will be querying for mode config

STD_CXXPPFLAGS = read_root_config("cxx_#default", "cxxppflags") or ""

@autozimu
Copy link
Contributor

Do you also have public_include_directories set on that target?

Yes it's set for this target.

    "public_include_directories": [],
    "public_system_include_directories": [
      "."
    ],

@zjturner
Copy link
Contributor Author

I haven't had time to come back to this yet, but do you think the issue could be because we're using "." as the header_namespace on ~100% of our targets? If I'm reading the code correctly, that disables the heuristic to map the buck-out path to the source tree.

@autozimu
Copy link
Contributor

I doubt that's the real reason, but you can go ahead modifying the header namespace processing part and see if it changes anything. Though I would be looking very closely at passed in attrs["exported_headers"], as the code doesn't do much if there is buck-out in passed in header path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants