On Tue, May 03, 2011 at 10:29:21PM -0700, Edward Pilatowicz wrote: > On Tue, May 03, 2011 at 05:30:15PM -0700, Brock Pytlik wrote: > > On 05/ 3/11 04:56 PM, Edward Pilatowicz wrote: > > >On Tue, May 03, 2011 at 01:28:06PM -0700, Brock Pytlik wrote: > > >>Webrev: > > >>http://cr.opensolaris.org/~bpytlik/ips-18240-v1/ > > >> > > >>Bug: > > >> 18240 zone proxy needed > > >> > > >looks much better, but i still have more makefile comments. > > >ed > > > > ... > > >--------- > > >src/pkg/manifests/package%252Fsysrepo.p5m > > > > > >- the zone proxy should probably be delivered in it's own package since > > > by default we don't need a sysrepo (and apache) installed in > > > non-global zones. the zone proxy package would then have a global > > > zone only dependancy on the sysrepo package. > > That doesn't seem right to me. I'm happy to add more variant tags to > > actions if you'd like, but I believe this all belongs in one > > package. > > why doesn't this seem right? both you and tim have told me how you feel > the pkgrepo sever is useful by itself to server up data potentially > outside of the zones context. (we left stuff in during the pkgrepo code > review for when it's run outside the context of zones.) > > so given that: > - by default we don't need or want the sysrepo installed inside zones. > (and we don't want it to pull in a bunch of stuff it depends on.) > and: > - the sysrepo is potentially useful as a standalone service. > > it really seems like it should be in a seperate package. if you just > tag all the sysrepo components as global zone only then we'll never be > able to install them into a zone. > > > > > > >--------- > > >src/zoneproxy/Makefile.constants > > > > > >- in general when creating makefiles that will be included in other > > > makefiles you don't want to mix variables definitions and dependancy > > > rules. there are multiple reason for this that i can explain in > > > person. i'd recommnd breaking this file up into Makefile.master and > > > Makefile.targ (with variables and rules in each file respectively). > > > Then in each of the subdirectory makfiles that include this makefile, > > > include Makefile.master at the top and Makefile.targ at the bottom. > > Is this really necessary before putback? Is something being built > > incorrectly? Are my binaries being produced with the wrong options? > > If not, I'd just as soon leave this alone. > > unless you're next putback will be fixing up these makefiles i'd > say these changes are necessary. makefiles are tricky to get right and > you're not going to maintaining these forever. (someone else is and > they'll have to figure out all the stuff that you did.) > > > >- add some common variables here for doing INSTALLs > > > FILEMODE= 0644 > > > DIRMODE= 0755 > > > INS.file= $(RM) -f $@; $(INS) -s -m $(FILEMODE) -f $(@D) $< > > > INS.dir= $(INS) -s -d -m $(DIRMODE) $@ > > > > > I think doing it the way it is preferable for now. > > currently you have multiple duplicated INSTALL rules in different > makefiles. how is duplicated code preferable? > > > >- the $(ZONES_PROG) rule shouldn't make the target directory. instead > > > do this via a generic dependency rule like: > > > > > > $(ZONES_LIBDIR): > > > $(INS.dir) > > > > > > $(ZONES_LIBDIR)/%: % $(ZONES_LIBDIR) > > > $(INS.file) > > See above. > > >- the $(ZONES_PROG) rule shouldn't update the $(PROG) binary. instead > > > the $(PROG) binary should be the same as the binary installed in the > > > proto area. so we should run mcs on the objects when building them. > > > to do that i'd recommend overriding the default build rules in your > > > Makefile.master by doing: > > > > > > .c: > > > $(LINK.c) -o $@ $< $(LDLIBS) > > > $(MCS) -d $@ > > > .c.o: > > > $(COMPILE.c) $(OUTPUT_OPTION) $< > > > $(MCS) -d $@ > > Huh, I thought this was exactly what you've been telling me not to > > do. But hey, ok, I'll do it this way now. > > i was telling you not to repeatedly define your own random rules ala: > > $(SOME_TARGET): $(SOME_DEP) > $(ONE_OFF_BUILD_RULE) > > by doing the overrides above you update the default rules and don't have > to constantly re-declare new rules like you were doing before. > > > >- you should put common rules for installing header files in here > > > instead of in zoneproxyd/Makefile. for example: > > > > > > ROOTHDRDIR= $(ROOT)/usr/include > > > ROOTHDRS= $(HDRS:%=$(ROOTHDRDIR)/%) > > > > > > $(ROOTHDRDIR): > > > $(INS.dir) > > > > > > $(ROOTHDRDIR)/%: % $(ROOTHDRDIR) > > > $(INS.file) > > I'd rather just leave things as they are. > > >- you can put common rules in here for install_h (and remove it from > > > zoneproxyd/Makefile): > > > > > > install_h: $(ROOTHDRS) > > This is working as is, so I'm going to leave it. > > but it is not semantically correct. we could define makefile rules > called goobaz and foobar and make them do whatever we want, but if we're > trying to stick to standard conventions (like install_h, all, install, > etc) we should make those rules perform as expected. > > > >--------- > > >src/zoneproxy/list/Makefile > > > > > >- you should be able to delete HDR_MARKER. > > > > > >- i'd recommend just moving these list* files back into the zoneproxyd > > > directory. > > I like them where they are, and was told to move them out, so I'm > > just going to leave them there since it's actually working. > > > > hm. well, because you have them in a parallel directory but you > haven't expressed that zoneproxyd has a dependency on list/list.o, then > if someone tries to run a parallel make (like me) they'll hit a race in > build list.o and zoneadmd and occasionally their builds will fail. > seems poor. > > i'd recommend you do one of the following: > > - move the list files into the zoneproxyd directory. > - add parallel directives or explicitly dependencies to the makefiles to > avoid races. > > ed
We seem to be coming to closure on all aspects of the zone proxy code review with the exception for the Makefile. Since the comments are around semantics and I haven't seen any concerns about the Makefiles not working correctly I'd suggest we move forward with landing this and filing a bug to address the Makefile concerns post putback. This will allow us to get the zone proxy wad into the build with the rest of the zones changes to enable us to test all of the new zones changes together. As I noted in the team meeting I'd like to get Bart's view, but I know he is incredibly swamped at this point. BJ -- B.J. Wahl Solaris Kernel Development _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
