Bug 3514

Mon Nov 17 18:44:15 UTC 2008  Simon McQueen  <sm@prismtech.com>

        * tao/PolicyS.h:

          Add a #include of tao/PortabServer/PolicyS.h.

        * tests/Bug_3513_Regression/test.cpp:

          Whitespace.

        * tests/Bug_3514_Regression:
        * tests/Bug_3514_Regression/Bug_3514_Regression.mpc:
        * tests/Bug_3514_Regression/README:
        * tests/Bug_3514_Regression/test.idl:
        * tests/Bug_3514_Regression/test.cpp:

          Regression test for the above fix.

Simon, this makes the core TAO lib dependent on TAO PortableServer, seems not the correct change.

reopening

Nothing includes "tao/PolicyS.h" that I can find by grepping the ORB core. How do you see the dependency occurring ?

ok, see your point, this is risky, including files from the portableserver in the core. if somebody does include tao/policys.h he now gets this dependency. 

would it be possible to get PolicyS.h pulled in when not wanted, maybe through messaging? when you make the derived interface local probably this compiles then fine without changes? Shouldn't the IDL compiler generate then an additional include when a non local interface is derived? So, not change the code, but handle this in the IDL compiler because this is a side effect of refactoring.

If somebody deliberately includes anything that ends *S.h then they are by definition including a server side header file and should expect to have to link the PortableServer library.

I have built Messaging. It does not include PolicyS.h.

I don't think that there is any risk but if you want to assign this to yourself and do it differently some way then be my guest - as long as my regression test still passes I'll be perfectly happy with anything you come up with. If not then please put the ticket back to fixed. Thanks.

(In reply to comment #7)
> If somebody deliberately includes anything that ends *S.h then they are by
> definition including a server side header file and should expect to have to
> link the PortableServer library.
> 
> I have built Messaging. It does not include PolicyS.h.
> 
> I don't think that there is any risk but if you want to assign this to yourself
> and do it differently some way then be my guest - as long as my regression test
> still passes I'll be perfectly happy with anything you come up with. If not
> then please put the ticket back to fixed. Thanks.

I think we shouldn't start this path, including files from sub directories, at the end it could do more harm then it solves now. I think in the past I tried todo the same and got similar remarks as I am making now. 

I think the best change is to make TAO_IDL aware of this, or add portableserver/policy.pidl. I don't have time to test changes, I am just trying to keep the core clean and easy to maintain, as you hopefully can understand. Include other header files just assuming that nobody includes that file maybe works at this moment, but could lead to issues for other users or when other work is done in the near feature.

Also, the PolicyC/S.h files really should go out of the repository, they should be generated by the IDL compiler, not as handcrafted file in the repo. This is one of the last files we have. Your handcrafted addition will then also not work anymore, another reason to solve this in a different way

> I think we shouldn't start this path, including files from sub directories, at
> the end it could do more harm then it solves now. I think in the past I tried
> todo the same and got similar remarks as I am making now. 
> 
> I think the best change is to make TAO_IDL aware of this, or add
> portableserver/policy.pidl. I don't have time to test changes, I am just trying
> to keep the core clean and easy to maintain, as you hopefully can understand.
> Include other header files just assuming that nobody includes that file maybe
> works at this moment, but could lead to issues for other users or when other
> work is done in the near feature.

The problem solved now is that the ORB did not compile spec compliant IDL. I would vote 'ugly and right' over 'pretty and broken' every time in these circumstances if a choice has to be made. If we could have 'pretty and right' that would be best - I understand and agree with you totally - but we both have too busy schedules at the minute for cosmetic changes.

As to what issues might or might not arise in the future, if we think of a problematic use case let's raise an issue now and add a test. It is easier once we have something concrete we can use to justify allocating some more resources.

> Also, the PolicyC/S.h files really should go out of the repository, they should
> be generated by the IDL compiler, not as handcrafted file in the repo. This is
> one of the last files we have. Your handcrafted addition will then also not
> work anymore, another reason to solve this in a different way

The good thing is that as I've added a regression test so we don't need to worry about this. When removing these cruft files gets to the top of someone's priority list then whoever is doing it will have assurance that they have done it in a way that works.

(In reply to comment #9)
> The problem solved now is that the ORB did not compile spec compliant IDL. I
> would vote 'ugly and right' over 'pretty and broken' every time in these
> circumstances if a choice has to be made. If we could have 'pretty and right'
> that would be best - I understand and agree with you totally - but we both have
> too busy schedules at the minute for cosmetic changes.

But if we make to many 'ugly and right' patches we do have the risk that future changes will take more and more time and that we get more and more ugly stuff. 

> As to what issues might or might not arise in the future, if we think of a
> problematic use case let's raise an issue now and add a test. It is easier once
> we have something concrete we can use to justify allocating some more
> resources.

Let us keep this ticket open as reminder that we should make a pretty and right change when time permits it. Maybe as last action we should check for any include of tao/PortableServer/PolicyS.h in code in the repo, maybe people have workaround it in the past