Re: [Scons-dev] Patch for review: add missing .itcl target to SWIG builder
Thanks William for the comments. The .itcl file is interpreted Tcl code, unlike Java/C/C++ it does not need to be built. Unfortunately I'm too busy to create the pull request right now :( I will add it to my backlog. The patch will also get some internal testing here (on scons 2.2.0), although I couldn't find another module in our source tree which uses the -itcl flag. Ben From: Scons-dev [mailto:scons-dev-boun...@scons.org] On Behalf Of William Blevins Sent: 06 November 2014 00:25 To: SCons developer list Subject: Re: [Scons-dev] Patch for review: add missing .itcl target to SWIG builder I have two concerns about this patch as it stands: 1) Compatibility impact on existing SConscript from changing the length of the returned target list. 2) The SWIG builder gets invoked through env.CFile() or env.CXXFile(), however only one of the returned targets is a C/C++ file. Unless you have a specific reason, I do not foresee either of these conditions causing issues. 1) Emitters return lists of elements by design, so this should be handled without concern. 2) Builders are chosen for Nodes per their scanner_key, so unless you explicitly add the ".itcl" extension to the CSuffixes list (or some other equivalent method), then the item will not be built with the C-builder action. If this file needs to be built in some way, then a build action needs to be created for it. The best way to get your patch looked at (sooner vs later) is to create a pull request @ https://bitbucket.org/scons/scons ; there should be some notes about the process @ http://www.scons.org/wiki/SconsMercurialWorkflows . V/R, William On Wed, Nov 5, 2014 at 4:35 PM, Ben Golding wrote: After reading the responses to the other thread that I opened, I now understand that SideEffect() should not be used for files that I want to install, or indeed care about at all. I found a couple of cases where the SWIG builder misses some generated files from the returned targets, in generating Java and [incr Tcl] code. The Java case is a bit complex, since there can be many additional files; figuring out the names would amount to writing a C/C++ parser. I've opened a thread on the SWIG users list about this. When specifying the -itcl option, however, the output file seems to be always based on the input (foo.i => foo.itcl). I've attached a simple patch which changes the emitter to return foo.itcl in target[1]. There is no change to target[0] which is a C/C++ file. I have two concerns about this patch as it stands: 1) Compatibility impact on existing SConscript from changing the length of the returned target list. 2) The SWIG builder gets invoked through env.CFile() or env.CXXFile(), however only one of the returned targets is a C/C++ file. I suppose the above could be addressed by extra code to handle the .itcl extension in generate(), but I'm unsure exactly the right way to do this. I'm also unsure how the SConscript should look in that case, since the .itcl and .cpp files are generated at once. (The .itcl file is something like a "side effect", but we do care about it.) So returning the additional list item from CXXFile() seems a pragmatic solution. Comments/improvements are welcome. Regards Ben ___ Scons-dev mailing list Scons-dev@scons.org https://pairlist2.pair.net/mailman/listinfo/scons-dev ___ Scons-dev mailing list Scons-dev@scons.org https://pairlist2.pair.net/mailman/listinfo/scons-dev
Re: [Scons-dev] Patch for review: add missing .itcl target to SWIG builder
> > I have two concerns about this patch as it stands: > 1) Compatibility impact on existing SConscript from changing the length of > the returned target list. > 2) The SWIG builder gets invoked through env.CFile() or env.CXXFile(), > however only one of the returned targets is a C/C++ file. Unless you have a specific reason, I do not foresee either of these conditions causing issues. 1) Emitters return lists of elements by design, so this should be handled without concern. 2) Builders are chosen for Nodes per their scanner_key, so unless you explicitly add the ".itcl" extension to the CSuffixes list (or some other equivalent method), then the item will not be built with the C-builder action. If this file needs to be built in some way, then a build action needs to be created for it. The best way to get your patch looked at (sooner vs later) is to create a pull request @ https://bitbucket.org/scons/scons ; there should be some notes about the process @ http://www.scons.org/wiki/SconsMercurialWorkflows . V/R, William On Wed, Nov 5, 2014 at 4:35 PM, Ben Golding wrote: > After reading the responses to the other thread that I opened, I now > understand that SideEffect() should not be used for files that I want to > install, or indeed care about at all. > > I found a couple of cases where the SWIG builder misses some generated > files from the returned targets, in generating Java and [incr Tcl] code. > > The Java case is a bit complex, since there can be many additional files; > figuring out the names would amount to writing a C/C++ parser. I've opened > a thread on the SWIG users list about this. > > When specifying the -itcl option, however, the output file seems to be > always based on the input (foo.i => foo.itcl). I've attached a simple patch > which changes the emitter to return foo.itcl in target[1]. There is no > change to target[0] which is a C/C++ file. > > I have two concerns about this patch as it stands: > 1) Compatibility impact on existing SConscript from changing the length of > the returned target list. > 2) The SWIG builder gets invoked through env.CFile() or env.CXXFile(), > however only one of the returned targets is a C/C++ file. > > I suppose the above could be addressed by extra code to handle the .itcl > extension in generate(), but I'm unsure exactly the right way to do this. > I'm also unsure how the SConscript should look in that case, since the > .itcl and .cpp files are generated at once. (The .itcl file is something > like a "side effect", but we do care about it.) So returning the additional > list item from CXXFile() seems a pragmatic solution. > > Comments/improvements are welcome. > > Regards > Ben > > ___ > Scons-dev mailing list > Scons-dev@scons.org > https://pairlist2.pair.net/mailman/listinfo/scons-dev > > ___ Scons-dev mailing list Scons-dev@scons.org https://pairlist2.pair.net/mailman/listinfo/scons-dev
[Scons-dev] Patch for review: add missing .itcl target to SWIG builder
After reading the responses to the other thread that I opened, I now understand that SideEffect() should not be used for files that I want to install, or indeed care about at all. I found a couple of cases where the SWIG builder misses some generated files from the returned targets, in generating Java and [incr Tcl] code. The Java case is a bit complex, since there can be many additional files; figuring out the names would amount to writing a C/C++ parser. I've opened a thread on the SWIG users list about this. When specifying the -itcl option, however, the output file seems to be always based on the input (foo.i => foo.itcl). I've attached a simple patch which changes the emitter to return foo.itcl in target[1]. There is no change to target[0] which is a C/C++ file. I have two concerns about this patch as it stands: 1) Compatibility impact on existing SConscript from changing the length of the returned target list. 2) The SWIG builder gets invoked through env.CFile() or env.CXXFile(), however only one of the returned targets is a C/C++ file. I suppose the above could be addressed by extra code to handle the .itcl extension in generate(), but I'm unsure exactly the right way to do this. I'm also unsure how the SConscript should look in that case, since the .itcl and .cpp files are generated at once. (The .itcl file is something like a "side effect", but we do care about it.) So returning the additional list item from CXXFile() seems a pragmatic solution. Comments/improvements are welcome. Regards Ben swig.py.patch Description: swig.py.patch ___ Scons-dev mailing list Scons-dev@scons.org https://pairlist2.pair.net/mailman/listinfo/scons-dev
