OpenDDS / OpenDDS

OpenDDS is an open source C++ implementation of the Object Management Group (OMG) Data Distribution Service (DDS). OpenDDS also supports Java bindings through JNI.
http://www.opendds.org
Other
1.29k stars 465 forks source link

Support for versioned namespaces #332

Closed jwillemsen closed 7 years ago

jwillemsen commented 7 years ago

New pull request with all necessary changes to enable versioned namespaces.

Did a local full compile with versioned namespaces enabled and now everything compiles.

mitza-oci commented 7 years ago

Did a local full compile with versioned namespaces enabled and now everything compiles.

Does it require pr #330 to be integrated first?

In file included from DdsDcpsCoreTypeSupportImpl.cpp:3:0:
DdsDcpsCoreTypeSupportImpl.h:1589:25: error: reference to ‘DDS’ is ambiguous
   bool operator()(const DDS::ParticipantBuiltinTopicData& v1, const DDS::Partic
                         ^
DdsDcpsCoreTypeSupportImpl.h:1585:15: note: candidates are: namespace DDS { }
 namespace DDS {
               ^
In file included from DdsDcpsCoreTypeSupportImpl.h:5:0,
                 from DdsDcpsCoreTypeSupportImpl.cpp:3:
DdsDcpsCoreC.h:86:1: note:                 namespace DDS_3_9_0::DDS { }
 {
 ^
jwillemsen commented 7 years ago

No, not needed, did compile error free on my system without that pr

jwillemsen commented 7 years ago

Compile my tree a few times today but no compile errors anymore when enabling versioned namespaces

mitza-oci commented 7 years ago

test this please (this should trigger Jenkins)

mitza-oci commented 7 years ago

MPC/config/opendds_face.mpb has an "avoids" for versioned_namespaces. In ACE, the feature is called versioned_namespace. We should be consistent here and that will also pick up the correct value from global.features.

If the $DDS_ROOT/FACE library is not going to be compatible with versioned namespaces, should the source code changes in there be reverted? Also the tests need to be conditionally excluded in dcps_tests.lst.

jwillemsen commented 7 years ago

Fixed avoids, at this moment it is not compatible, couldn't get it working, but I will add a note for this, will try that again the coming weeks.

mitza-oci commented 7 years ago

I'm still seeing the same error I posted yesterday. I have the latest DOCGroup/ACE_TAO.git master built with versioned namespaces (Ubuntu 16.04 x86_64 default GCC). I built this PR pointed to that ACE+TAO and it failed with the namespace-related error I posted before.

jwillemsen commented 7 years ago

Strange, I assume you regenerated all makefiles and compile ACE+TAO? Not sure what is happening, tested this on a few systems here.

jwillemsen commented 7 years ago

On line 1582 and further I have

OPENDDS_BEGIN_VERSIONED_NAMESPACE_DECL
namespace DDS {
// This structure supports use of std::map with a key
// defined by one or more #pragma DCPS_DATA_KEY lines.
struct OpenDDS_Dcps_Export ParticipantBuiltinTopicData_OpenDDS_KeyLessThan {
  bool operator()(const DDS::ParticipantBuiltinTopicData& v1, const DDS::ParticipantBuiltinTopicData& v2) const
  {
    using ::operator<; // TAO::String_Manager's operator< is in global NS
    if (v1.key.value[0] < v2.key.value[0]) return true;
    if (v2.key.value[0] < v1.key.value[0]) return false;
    if (v1.key.value[1] < v2.key.value[1]) return true;
    if (v2.key.value[1] < v1.key.value[1]) return false;
    if (v1.key.value[2] < v2.key.value[2]) return true;
    if (v2.key.value[2] < v1.key.value[2]) return false;
    return false;
  }
};
}
OPENDDS_END_VERSIONED_NAMESPACE_DECL
mitza-oci commented 7 years ago

I'm not seeing those namespace lines. I verified that this was a completely fresh build using "git clean". Please see log file at gist

jwillemsen commented 7 years ago

The invocations of opendds_idl on these core IDL files lack arguments, are the makefiles generated, I see

/home/johnny/ACE/OpenDDS/bin/opendds_idl -Sa -St -Wb,pch_include=DCPS/DdsDcps_pch.h -Wb,export_macro=OpenDDS_Dcps_Export -Wb,export_include=dds/DCPS/dcps_export.h -Wb,versioning_begin=OPENDDS_BEGIN_VERSIONED_NAMESPACE_DECL -Wb,versioning_end=OPENDDS_END_VERSIONED_NAMESPACE_DECL DdsDcpsCore.idl Where yours has /tao_builds/mitza/versioned-OpenDDS/bin/opendds_idl -Sa -St -Wb,pch_include=DCPS/DdsDcps_pch.h -Wb,export_macro=OpenDDS_Dcps_Export -Wb,export_include=dds/DCPS/dcps_export.h DdsDcpsCore.idl

mitza-oci commented 7 years ago

OK, looks like it's getting further. I thought those arguments were already there because the generated header had another "begin namespace" just a few lines down.

Looks like we have a 'RepoIdGenerator' class that's in the global namespace.

mitza-oci commented 7 years ago

test this please Restarting Jenkins now that other PRs are merged

mitza-oci commented 7 years ago

Once this is ready to go I wonder if it would be a good use for the squash commit option in GitHub. If we do that, I assume the branch jwillemsen:jwi-versioned would not be used going forward.

jwillemsen commented 7 years ago

I would be in favor of a regular merge commit

jwillemsen commented 7 years ago

Looks one test fails but could that be jitter?