[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/libhip into lp:hipl

  • From: Diego Biurrun <diego@xxxxxxxxxx>
  • To: hipl-dev@xxxxxxxxxxxxx
  • Date: Mon, 13 Feb 2012 12:33:40 +0100

On Mon, Feb 13, 2012 at 12:23:03PM +0200, Xin Gu wrote:
> On 13/02/12 11:25, Diego Biurrun wrote:
> >On Mon, Feb 13, 2012 at 10:39:38AM +0200, Xin Gu wrote:
> >>On 13/02/12 00:26, Diego Biurrun wrote:
> >>>On Sun, Feb 12, 2012 at 10:33:00PM +0200, Xin Gu wrote:
> >>>>On 12/02/12 15:25, Diego Biurrun wrote:
> >>>>>On Fri, Feb 10, 2012 at 02:24:28PM +0000, Xin wrote:
> >>>>>>Xin has proposed merging lp:~hipl-core/hipl/libhip into lp:hipl.
> >>>>>>
> >>>>>>@@ -244,21 +255,24 @@
> >>>>>>
> >>>>>>  ### static library dependencies ###
> >>>>>>
> >>>>>>-hipd_hipd_LDADD                          = lib/core/libhipcore.la
> >>>>>>-hipfw_hipfw_LDADD                        = lib/core/libhipcore.la
> >>>>>>+hipd_hipd_LDADD                          = 
> >>>>>>lib/hipdaemon/libhipdaemon.la
> >>>>>>+hipfw_hipfw_LDADD                        = 
> >>>>>>lib/hipdaemon/libhipdaemon.la
> >>>>>Why does the firewall depend on libhipdaemon?
> >>>>Because modules/midauth/hipd/midauth.c is used both for building fw
> >>>>and libdaemon. Well, I am not an autotool expert, perhaps it is not
> >>>>the best solution.
> >>>This is not related to autotools at all.  Suddenly you need to link
> >>>the firewall against all of hipd.  What symbols did you move and why
> >>>did you move it to the libhipdaemon code and not to the common code.
> >>In trunk, the modules/midauth/hipd/midauth.c is used to build both
> >>hipd and hipfw.
> >No.
> 
> My unreliable memory... The file causes the conflict should be:
> modules/midauth/lib/midauth_builder.c, sorry

And that was a *huge* bug that you amplified a hundredfold in this
merge request.  Fixed.  hipfw does not and must not depend on hipd
code.  Your build system changes must reflect this and you must not
make hipfw depend on hipd with "autotools" handwaving.

> >>>>>>--- hipd/esp_prot_hipd_msg.c    2011-12-16 13:37:33 +0000
> >>>>>>+++ lib/hipdaemon/esp_prot_hipd_msg.c   2012-02-10 14:23:36 +0000
> >>>>>>@@ -57,6 +57,10 @@
> >>>>>>
> >>>>>>+int  esp_prot_active               = 0;
> >>>>>>+int  esp_prot_num_transforms       = 0;
> >>>>>>+long esp_prot_num_parallel_hchains = 0;
> >>>>>This looks suspicious.  Why are you moving these global variables, but
> >>>>>not the extern declarations?
> >>>>>
> >>>>>Why is this part of this merge request and split off and done separately?
> >>>>>The same applies to all the other similar changes.  I'm not convinced you
> >>>>>need to do them (here).
> >>>>Because HIPL daemon and libhip both link to libhipdaemon.la, but
> >>>>hipd.c is not part of libhipdaemon, so those global variables have
> >>>>been moved to other files otherwise libhip can not access them.
> >>>Why did you move them to these files in the first place?
> >>Those moves come from previous work in libhip branch. I think it is
> >>reasonable, if you consider to share code between hipd and libhip.
> >If those moves make sense at all, then they make sense outside of this
> >merge request and should be submitted separately.
> 
> But before we have this libhip staff, we didn't have the requirement
> to share those codes. IMO keeping them in merge can help us to track
> why we move those variables in the future.

We have Bazaar to track why me make changes.

I can only repeat what I already said: If those moves make sense at all,
then they make sense outside of this merge request and should be submitted
separately.

Diego

Other related posts: