Skip to content

Conversation

@mlalic
Copy link
Contributor

@mlalic mlalic commented Feb 21, 2020

Generated component headers (.g.h) and C++ source files (.g.cpp)
are emitted into a directory structure that mirrors the
namespace that the WinRT component is found in.

For instance, for a component such as:

namespace Foo.Bar
{
    runtimeclass Baz
    {
        void Frob();
    }:
}

The .g.h and .g.cpp files will be emitted into a newly created
subdirectory Foo/Bar of the output directory given to cppwinrt.exe
as the -out parameter.

Meanwhile, the component source file (.cpp file, meant to be edited by
the user implementing the component), emits a #include that assumes
that the .g.cpp file will be emitted directly in the output directory as
a fully-qualified name of the file. Specifically, the include that is
emitted is:

#include "Foo.Bar.Baz.g.cpp"

While the file Foo.Bar.Baz.g.cpp doesn't even exist. In reality the
file that exists and that should be included is:

#include "Foo/Bar/Baz.g.cpp"

The component header (.h file, meant to be edited by the user
implementing the component) already emits the correct include path for
the .g.h file that is emitted in the same way as the .g.cpp file is.

Note that this is typically not an issue for users who specify a -name
parameter to cppwinrt.exe, which strips the fully qualified namespace
from generated file names and doesn't cause the directory structure
to be created. It is an issue if -name isn't specified or if
cppwinrt.exe is unable to strip the namespace from all types that are
found in the input winmds.


This commit fixes the issue by using the get_generated_component_filename
helper to synthesize the name of the file that should be included when
the .g.cpp file is included. This follows the approach that the
component header already uses when emitting the include for the .g.h file.

Generated component headers (.g.h) and C++ source files (.g.cpp)
are emitted into a _directory structure_ that mirrors the
namespace that the WinRT component is found in.

For instance, for a component such as:

```
namespace Foo.Bar
{
    runtimeclass Baz
    {
        void Frob();
    }:
}
```

The .g.h and .g.cpp files will be emitted into a newly created
subdirectory `Foo/Bar` of the output directory given to `cppwinrt.exe`
as the `-out` parameter.

Meanwhile, the component source file (.cpp file, meant to be edited by
the user implementing the component), emits a `#include` that assumes
that the .g.cpp file will be emitted directly in the output directory as
a fully-qualified name of the file. Specifically, the include that is
emitted is:

```
```

While the file `Foo.Bar.Baz.g.cpp` doesn't even exist. In reality the
file that exists and that should be included is:

```
```

The component header (.h file, meant to be edited by the user
implementing the component) already emits the correct include path for
the .g.h file that is emitted in the same way as the .g.cpp file is.

Note that this is typically not an issue for users who specify a `-name`
parameter to `cppwinrt.exe`, which strips the fully qualified namespace
from both generated file names and doesn't cause the directory structure
to be created. It is an issue if `-name` isn't specified or if
`cppwinrt.exe` is unable to strip the namespace from all types that are
found in the input `winmd`s.

---

This commit fixes the issue by using the `get_generated_component_filename`
helper to synthesize the name of the file that should be included when
the .g.cpp file is included. This follows the approach that the
component header uses.
@mlalic mlalic changed the title Fix include path for generated component (.g.cpp files) Fix include path for generated component (.g.cpp) files Feb 21, 2020

if (settings.component_opt)
{
auto filename = get_generated_component_filename(type);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tested with or without -prefix? Any change like would need to be justified in detail and then tested very heavily as there are a lot of projects publicly and inside the Windows OS that rely on historical quirky behavior related to include paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have tested with and without -prefix. This change works correctly in both cases. (When -prefix is specified, it still emits the include in the form of Foo.Bar.Baz.g.cpp, as expected and as before the change.) The reason it works is because get_generated_component_filename conditionally generates the directory path or the dotted prefix file based on whether the option is provided or not.

I think the description is already pretty detailed, to be honest, but I am happy to add more information that you feel might be missing.

The crux of the issue here isn't really the include path, but simply the file name that is emitted in the include directive is never created (if -prefix isn't specified and -name is unable to fully strip the namespace prefix). The actual file name ends up being Baz.g.cpp, but we're emitting an include for Foo.Bar.Baz.g.cpp. That's clearly not going to work regardless of include path configuration. :) And I discovered this in the OS repo itself, where I had to work around it by manually modifying the emitted include path :)

Further, this is a change to what's emitted to a file that is already meant to be edited manually (we now even have a static_assert to that effect) so risk of breaking any existing project that uses cppwinrt is extremely low.

Finally, as I wrote in the description, this uses the exact same function to generate the included file name as is used to emit the include for the .g.h file in the component's .h file. .g.cpp files are generated using the same code as .g.h files and wind up in the same location and therefore should use the same approach to emit the include.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent, thanks for the due diligence. Obviously I'm all for fixing broken scenarios. Just cautious about breaking existing ones. We just don't have sufficient test collatoral to catch breaks early and its a lot more painful to catch them when a branch build breaks (as I'm sure you're aware).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, of course. :)

@kennykerr
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

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

Thanks!

@kennykerr kennykerr merged commit 302097f into microsoft:master Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants