Re: Builder._createNodes appears to handle slist incorrectly
Bill Deegan <bill-cJFiu+DHMVC5azolltMz9laTQe2KTcn/@public.gmane.org> Sun, 11 Sep 2022 21:39:52 -0700
| Newsgroups | gmane.comp.programming.tools.scons.devel |
|---|---|
| Message-ID | <CAEyG4CFwLCEoL5NWGuRdNJqc83nhSta-Hy=yGqg_ED5QcH5OYQ@mail.gmail.com> |
--===============7648808433285639193== Content-Type: multipart/alternative; boundary="0000000000003eef2505e8737de4" --0000000000003eef2505e8737de4 Content-Type: text/plain; charset="UTF-8" I'd put money on it not being broken and your usage perhaps not quite right.. There's a lot of code in your PR. You might try adding `multi=True` when you create your builder. That said having VcPkg as a builder may not be the best way to implement. You might be better off with a PseudoBuilder. I'll add some comments in the PR when I get some time to do a thorough review. On Sun, Sep 11, 2022 at 6:08 PM Ryan Saunders <[email protected]> wrote: > Hi guys, > > While adding some unit tests to my VCPkg builder, I thought to try the > "list" case, where someone does (e.g.) "env.VCPkg(["openjpeg", "glut"])" to > install a list of packages. I was surprised to discover that this doesn't > work...my source type's target_from_source method only gets called for the > first item in the list, and the subsequent source nodes get dropped in the > trash can, with no corresponding "target" nodes created for them. > > I tracked the issue down to this code: > def _create_nodes(self, env, target = None, source = None): > """Create and return lists of target and source nodes. > """ > src_suf = self.get_src_suffix(env) > > target_factory = env.get_factory(self.target_factory) > source_factory = env.get_factory(self.source_factory) > > source = self._adjustixes(source, None, src_suf) > slist = env.arg2nodes(source, source_factory) > > pre = self.get_prefix(env, slist) > suf = self.get_suffix(env, slist) > > * if target is None:* > * try:* > * t_from_s = slist[0].target_from_source* > * except AttributeError:* > * raise UserError("Do not know how to create a target from > source `%s'" % slist[0])* > * except IndexError:* > * tlist = []* > * else:* > * splitext = lambda S: self.splitext(S,env)* > * tlist = [ t_from_s(pre, suf, splitext) ]* > > I think this last bit ought to be: > * if target is None:* > * tlist = []* > * for s in slist:* > * try:* > * t_from_s = s.target_from_source* > * except AttributeError:* > * raise UserError("Do not know how to create a target > from source `%s'" % s)* > * else:* > * splitext = lambda S: self.splitext(S,env)* > * tlist += [ t_from_s(pre, suf, splitext) ]* > > Does anyone know why this code is the way it is? Fixing this code seems a > tiny bit scary, since it's so fundamental... Then again, it seems > fundamentally broken. > > If I were to fix this, where would the best spot be for the appropriate > unit test? Any special considerations I should be aware of? > > R > > Sent with Proton Mail <https://proton.me/> secure email. > _______________________________________________ > Scons-dev mailing list > [email protected] > https://pairlist2.pair.net/mailman/listinfo/scons-dev > --0000000000003eef2505e8737de4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div>I'd put money on it not being broken and your usa= ge perhaps not quite right..</div><div><br></div><div>There's a lot of = code in your PR.</div><div><br></div><div>You might try adding `multi=3DTru= e` when you create your builder.</div><div>That said having VcPkg as a buil= der may not be the best way to implement.</div><div>You might be better off= with a PseudoBuilder.<br></div><div><br></div><div>I'll add some comme= nts in the PR when I get some time to do a thorough review.<br></div></div>= <br><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Sun= , Sep 11, 2022 at 6:08 PM Ryan Saunders <<a href=3D"mailto:business@jedi= ry.com">[email protected]</a>> wrote:<br></div><blockquote class=3D"gm= ail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,= 204,204);padding-left:1ex"><div style=3D"font-family:Arial;font-size:14px">= <span style=3D"line-height:1.5">Hi guys,</span><div style=3D"line-height:1.= 5"><br></div><div style=3D"line-height:1.5">While adding some unit tests to= my VCPkg builder, I thought to try the "list" case, where someon= e does (e.g.) "env.VCPkg(["openjpeg", "glut"])&quo= t; to install a list of packages. I was surprised to discover that this doe= sn't work...my source type's target_from_source method only gets ca= lled for the first item in the list, and the subsequent source nodes get dr= opped in the trash can, with no corresponding "target" nodes crea= ted for them.</div><div style=3D"line-height:1.5"><br></div><div style=3D"l= ine-height:1.5">I tracked the issue down to this code:</div><div style=3D"l= ine-height:1.5"><span>=C2=A0 =C2=A0 def _create_nodes(self, env, target =3D= None, source =3D None):</span><div><span>=C2=A0 =C2=A0 =C2=A0 =C2=A0 "= ;""Create and return lists of target and source nodes.</span></di= v><div><span>=C2=A0 =C2=A0 =C2=A0 =C2=A0 """</span></div><di= v><span>=C2=A0 =C2=A0 =C2=A0 =C2=A0 src_suf =3D self.get_src_suffix(env)</s= pan></div><div><br></div><div><span>=C2=A0 =C2=A0 =C2=A0 =C2=A0 target_fact= ory =3D env.get_factory(self.target_factory)</span></div><div><span>=C2=A0 = =C2=A0 =C2=A0 =C2=A0 source_factory =3D env.get_factory(self.source_factory= )</span></div><div><br></div><div><span>=C2=A0 =C2=A0 =C2=A0 =C2=A0 source = =3D self._adjustixes(source, None, src_suf)</span></div><div><span>=C2=A0 = =C2=A0 =C2=A0 =C2=A0 slist =3D env.arg2nodes(source, source_factory)</span>= </div><div><br></div><div><span>=C2=A0 =C2=A0 =C2=A0 =C2=A0 pre =3D self.ge= t_prefix(env, slist)</span></div><div><span>=C2=A0 =C2=A0 =C2=A0 =C2=A0 suf= =3D self.get_suffix(env, slist)</span></div><div><br></div><div><span><b>= =C2=A0 =C2=A0 =C2=A0 =C2=A0 if target is None:</b></span></div><div><span><= b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 try:</b></span></div><div><span= ><b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 t_from_s =3D sl= ist[0].target_from_source</b></span></div><div><span><b>=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 except AttributeError:</b></span></div><div><span>= <b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise UserError(= "Do not know how to create a target from source `%s'" % slist= [0])</b></span></div><div><span><b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 except IndexError:</b></span></div><div><span><b>=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tlist =3D []</b></span></div><div><span>= <b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else:</b></span></div><div><sp= an><b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 splitext =3D = lambda S: self.splitext(S,env)</b></span></div><div><span><b>=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tlist =3D [ t_from_s(pre, suf, sp= litext) ]</b></span></div><b><span></span></b><br></div><div style=3D"line-= height:1.5">I think this last bit ought to be:</div><div style=3D"line-heig= ht:1.5"><span><b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 if target is None:</b></span><= div><span><b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tlist =3D []</b></sp= an></div><div><span><b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for s in s= list:</b></span></div><div><span><b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 try:</b></span></div><div><span><b>=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 t_from_s =3D s.target_from= _source</b></span></div><div><span><b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 except AttributeError:</b></span></div><div><span><b>= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise= UserError("Do not know how to create a target from source `%s'&qu= ot; % s)</b></span></div><div><span><b>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 else:</b></span></div><div><span><b>=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 splitext =3D lambda S:= self.splitext(S,env)</b></span></div><div><span><b>=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tlist +=3D [ t_from_s(pre,= suf, splitext) ]</b></span></div><b><span></span></b><br></div><div style= =3D"line-height:1.5">Does anyone know why this code is the way it is? Fixin= g this code seems a tiny bit scary, since it's so fundamental... Then a= gain, it seems fundamentally broken.</div><div style=3D"line-height:1.5"><b= r></div><div style=3D"line-height:1.5">If I were to fix this, where would t= he best spot be for the appropriate unit test? Any special considerations I= should be aware of?</div><div style=3D"line-height:1.5"><br></div><span st= yle=3D"line-height:1.5">R</span><br></div><div style=3D"font-family:Arial;f= ont-size:14px"><br></div> <div style=3D"font-family:Arial;font-size:14px"> <div></div> =20 <div> Sent with <a href=3D"https://proton.me/" rel=3D"noopener noreferrer= " target=3D"_blank">Proton Mail</a> secure email. </div> </div> _______________________________________________<br> Scons-dev mailing list<br> <a href=3D"mailto:[email protected]" target=3D"_blank">[email protected]= g</a><br> <a href=3D"https://pairlist2.pair.net/mailman/listinfo/scons-dev" rel=3D"no= referrer" target=3D"_blank">https://pairlist2.pair.net/mailman/listinfo/sco= ns-dev</a><br> </blockquote></div> --0000000000003eef2505e8737de4-- --===============7648808433285639193== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Scons-dev mailing list [email protected] https://pairlist2.pair.net/mailman/listinfo/scons-dev --===============7648808433285639193==--