New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 702397 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Difficulty linking ash struct traits unit tests.

Project Member Reported by msw@chromium.org, Mar 16 2017

Issue description

Difficulty linking ash struct traits unit tests.

This came up while adding shelf_struct_traits_unittest.cc in Patch Set 3 here:
  https://codereview.chromium.org/2750463009/#ps40001

For some reason, including ":ash_public_cpp", in the new unit_tests target isn't sufficient to pull in the struct traits definitions; I get these ash_unittests linker errors from that CL:

[1/1] LINK ./ash_unittests
FAILED: ash_unittests 
...
../../mojo/public/cpp/bindings/lib/serialization.h:89: error: undefined reference to 'ash::mojom::internal::ShelfItem_Data::Validate(void const*, mojo::internal::ValidationContext*)'
gen/ash/public/interfaces/shelf.mojom-shared.h:1183: error: undefined reference to 'mojo::StructTraits<ash::mojom::ShelfItemDataView, ash::ShelfItem>::Read(ash::mojom::ShelfItemDataView, ash::ShelfItem*)'
gen/ash/public/interfaces/shelf.mojom-shared.h:1107: error: undefined reference to 'mojo::StructTraits<ash::mojom::ShelfItemDataView, ash::ShelfItem>::image(ash::ShelfItem const&)'
gen/ash/public/interfaces/shelf.mojom-shared.h:1112: error: undefined reference to 'mojo::StructTraits<ash::mojom::ShelfItemDataView, ash::ShelfItem>::app_id(ash::ShelfItem const&)'
gen/ash/public/interfaces/shelf.mojom-shared.h:1115: error: undefined reference to 'mojo::StructTraits<ash::mojom::ShelfItemDataView, ash::ShelfItem>::title(ash::ShelfItem const&)'
gen/ash/public/interfaces/shelf.mojom-shared.h:1135: error: undefined reference to 'mojo::StructTraits<ash::mojom::ShelfItemDataView, ash::ShelfItem>::type(ash::ShelfItem const&)'
gen/ash/public/interfaces/shelf.mojom-shared.h:1137: error: undefined reference to 'mojo::StructTraits<ash::mojom::ShelfItemDataView, ash::ShelfItem>::image(ash::ShelfItem const&)'
gen/ash/public/interfaces/shelf.mojom-shared.h:0: error: undefined reference to 'mojo::StructTraits<ash::mojom::ShelfItemDataView, ash::ShelfItem>::shelf_id(ash::ShelfItem const&)'
gen/ash/public/interfaces/shelf.mojom-shared.h:1148: error: undefined reference to 'mojo::StructTraits<ash::mojom::ShelfItemDataView, ash::ShelfItem>::status(ash::ShelfItem const&)'
gen/ash/public/interfaces/shelf.mojom-shared.h:1149: error: undefined reference to 'mojo::StructTraits<ash::mojom::ShelfItemDataView, ash::ShelfItem>::app_id(ash::ShelfItem const&)'

If I add a direct unit_tests dependency on "//ash/public/interfaces:interfaces_internal", that works (at least for my debug component build).
I'll see if that's a valid workaround, but Yuzhu points out that it might not work for non-component builds...

We found one potential idea that might point towards a better solution, by exporting ShelfItem_Data in the generated file out/Default/gen/ash/public/interfaces/shelf.mojom-shared-internal.h
<...>
#define ASH_PUBLIC_IMPLEMENTATION 1
#include "ash/public/cpp/ash_public_export.h"
<...>
class ASH_PUBLIC_EXPORT ShelfItem_Data
 

Comment 1 by yzshen@chromium.org, Mar 16 2017

Cc: roc...@chromium.org
Components: -Internals>Mojo Internals>Mojo>Bindings
I think the reason is that mojom::Foo::Deserialize() calls into mojom::internal::Foo_Data::Validate() which is not exported. We cannot directly apply the same export macro as mojom::Foo, because this mojom::internal::Foo_Data is shared by mojom::Foo and mojom::blink::Foo, which may not use the same export macro.

Two possible solution:
1) have a "forwarder" of mojom::internal::Foo_Data for both the chromium variant and blink variant, and export those.
2) change the shared_cpp_sources to a component itself, and use its own export macros.

I am going to try out (1) first.

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e9919293913d03d2169f99c882bae1569902ea7e

commit e9919293913d03d2169f99c882bae1569902ea7e
Author: yzshen <yzshen@chromium.org>
Date: Fri Mar 17 16:29:58 2017

Mojo C++ Bindings: fix mojom::Foo::Deserialize() for component build.

Without this fix, if the mojom target containing mojom::Foo is linked into a
component, mojom::Foo::Deserialize() doesn't work outside of that component.

The reason is that the method calls mojom::Foo_Data::Valiate() which is not
exported.

BUG= 702397 

Review-Url: https://codereview.chromium.org/2754003005
Cr-Commit-Position: refs/heads/master@{#457791}

[modify] https://crrev.com/e9919293913d03d2169f99c882bae1569902ea7e/mojo/public/cpp/bindings/lib/serialization.h
[modify] https://crrev.com/e9919293913d03d2169f99c882bae1569902ea7e/mojo/public/cpp/bindings/tests/BUILD.gn
[modify] https://crrev.com/e9919293913d03d2169f99c882bae1569902ea7e/mojo/public/cpp/bindings/tests/struct_unittest.cc
[modify] https://crrev.com/e9919293913d03d2169f99c882bae1569902ea7e/mojo/public/interfaces/bindings/tests/BUILD.gn
[add] https://crrev.com/e9919293913d03d2169f99c882bae1569902ea7e/mojo/public/interfaces/bindings/tests/test_export2.mojom
[modify] https://crrev.com/e9919293913d03d2169f99c882bae1569902ea7e/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
[modify] https://crrev.com/e9919293913d03d2169f99c882bae1569902ea7e/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl

Comment 3 by yzshen@chromium.org, Mar 17 2017

Status: Fixed (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00d67ef0786f6a28b6557e6224ddce2104ce8207

commit 00d67ef0786f6a28b6557e6224ddce2104ce8207
Author: msw <msw@chromium.org>
Date: Fri Mar 17 18:24:01 2017

Fix struct traits test deps and target visibility.

Yuzhu exported mojom::Foo::Deserialize() for component builds:
  https://codereview.chromium.org/2754003005/
This removes symptomatic extra deps and visibility.

BUG= 702397 
TEST=ash_unittests still builds and links.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2758813002
Cr-Commit-Position: refs/heads/master@{#457820}

[modify] https://crrev.com/00d67ef0786f6a28b6557e6224ddce2104ce8207/ash/public/cpp/BUILD.gn
[modify] https://crrev.com/00d67ef0786f6a28b6557e6224ddce2104ce8207/ash/public/interfaces/BUILD.gn

Sign in to add a comment