All, I had to do another pull request because bitbucket couldn't figure out how to correctly update my previous pull request.
Please see https://bitbucket.org/scons/scons/pull-requests/244/issue-2264-cross-language-scanner-support/diff The latest request includes the update the Install performance issue. I have some questions as comments on the file diff, but this should be ready for an initial announcement, so that interested parties can take a peek. V/R, William On Wed, Jun 3, 2015 at 7:31 PM, William Blevins <[email protected]> wrote: > It seems that the current consensus is to update the tests to catch the > additional stdout for Case A or update how the stdout is parsed so > that changes to this behavior only hit the Install test and not other tests > that happen to call the Install command. This will affect about 10 tests > (1-2 lines each) since all the builder tests are more or less copy-pastes > of each other.... > On Wed, Jun 3, 2015 at 6:09 PM, Kenny, Jason L <[email protected]> > wrote: > >> << If I understand your vote correctly, you want Install targets >> non-recursive; thus, fixing Case A and removing test coverage of Case B. >> >> >> >> I would vote that all scanner should be recursive. >> >> >> >> From what I read on the B case is that it fails because it wants to >> non-recusive scanner. ( maybe I read this wrong.. but the test passes with >> your fix when before it failed… ie the test was expected to fail to succeed) >> > > It works with and without my patch. Case B tests do not pass with > non-recursive Install builder scanning. Since we are in agreement to let > Case A behave as is then Case B tests will continue to be happy campers. > > >> Both case A and case B work (ie the way I think they should) because of >> recursive scans. The issue of A being copied if B changes I understand as >> being “wasteful” because it was copied ( using hard links helps with >> this..ie why I have ccopy() in parts) but if this case was a.h to a.h.l was >> a tool running to make a.h.l then it would not be as B.h could have >> affected the output of a.h.l. It was just convenient that coping does not >> have this issue, I think making the general scanner in some way so Scons >> would not copy A.h would by over optimizing (ie to special case). >> >> >> >> I don’t see any case in which install should forget anything. The two >> reasons for install are to define node to be packages and to make an >> install sandbox for safe development work. Any changes in the build we >> would want reflexed in the install sandbox. Install does not use the >> Require() logic which is to say it expect it to exist but don’t rebuild me >> if the source changes. >> >> >> >> As a side not one item that I have with SCons is the copy actions. I have >> in general had to move to have a common copy builder ( ie the CCopy in >> Parts) to allow consistent and constant behavior when coping items ( ie >> directory or files). I even override the install builder in SCons to help >> this as the last round of issue with “installing” a directory and Scons >> calling the hidden mkdir actions for a node and saying it was done ( when >> none of the contents had been built or copied) I only bring this up as the >> issue in my mind with scanner are twofold: >> >> >> >> 1) Some issues with recursion ( which you are addressing) >> >> 2) Some issue with complex subst() with path variable not being >> handed correctly ( I fixed this I Parts) >> >> >> >> I don’t believe we want by default scons to make possible incorrect >> rebuilds or failed fist pass builds because of on purpose reduce dependency >> scanners. The issue with copy vs install I did not ever understand as copy >> and forget. That seems to suggest to me that you would have a built that >> might be bad as some bit did not get updated. Who would want that as a >> develop? I have to deal with that hair pulling time wasting annoyance when >> forced to use autoconf/make already. >> >> >> >> I do believe you are making a case indirectly that we should have a >> builder and that can take a scanner that would say the this dependent files >> that are returned are Require() so they exist, but don’t force rebuild vs >> the current logic of rebuild the target logic. I don’t believe we have that >> concept at the moment define as part of the builder in SCons. Maybe that >> should be added instead? I think that would solve the problem with A vs B >> and make SCons generally more useful. >> >> >> >> Jason >> >> >> >> >> >> >> >> *From:* Scons-dev [mailto:[email protected]] *On Behalf Of *William >> Blevins >> *Sent:* Wednesday, June 3, 2015 4:26 PM >> >> *To:* SCons developer list >> *Subject:* Re: [Scons-dev] SCons cross-language support >> >> >> >> >> >> On Wed, Jun 3, 2015 at 4:53 PM, Kenny, Jason L <[email protected]> >> wrote: >> >> I believe the copy of A is similar to linking on many systems. The linker >> runs but it doesn’t have to as the import libs/so did not change, SCons >> just assumes that if the dependent source changed everything above it must >> be redone as well. I think this was a fix made back in the 0.98 days as at >> one point this did not happen. I recall finding the code in the node that >> controlled this. So for me this is not a scanner issue but a node issue in >> that an actions should only go off if the sources are really different ( or >> the function that says it different is true). In this case A.h still would >> copy as B changed, but if we had a alpha.h that depends on A.h, it would >> not copy as A.h MD5 was not different. Currently SCons would copy the >> alpha.h. Such a change for example would also allow Parts which uses >> hardlinks by default to be a lot faster as I would not have scons >> rebuilding hard links redundantly when coping files. >> >> >> >> I agree here. Install builder targets do not have binfo objects or at >> least not MD5sum components. This is a bug IMHO. There are several >> related tigris issues if I remember correctly. Not sure about Copy targets. >> >> >> >> I am unclear on how removing case B would make A.h with the current patch >> not copy A.h is there some difference in a node being different because it >> is an implicit dependency ( ie scanner) vs that of being a source to a >> builder? >> >> >> >> I would add a source_scanner to the Install builder that was >> non-recursive, which would then break Case B, since it expects recursive >> scanning of Install targets. A quick examination of the test case I >> mentioned in the patch comments may be easier to understand than plain text >> which regard to Case B expectations. >> >> >> >> What is the difference in the use case for Install vs copy? My view was >> that Install is just a copy that adds the items to the known installed >> items for packaging calls later. >> >> >> >> I'm not 100% sure here. I think they both call copy_func, but their >> setup is slightly different. I just imagine that Install and Copy have >> different semantic meanings. Install is like a Copy + forget whereas Copy >> should track the target as a full node. >> >> >> >> Either way I agree, if not for different reason, the use case B should >> not be support. >> >> >> >> If I understand your vote correctly, you want Install targets >> non-recursive; thus, fixing Case A and removing test coverage of Case B. >> >> >> >> Jason >> >> >> >> *From:* Scons-dev [mailto:[email protected]] *On Behalf Of *William >> Blevins >> *Sent:* Wednesday, June 3, 2015 12:09 PM >> >> >> *To:* SCons developer list >> *Subject:* Re: [Scons-dev] SCons cross-language support >> >> >> >> To be clear, Case A and Case B are related. >> >> >> >> I can resolve one issue but not both, so its really a multiple choice >> question. >> >> On Wed, Jun 3, 2015 at 9:47 AM, Kenny, Jason L <[email protected]> >> wrote: >> >> My take is that the scanner changes should improve the ability for the >> scanner to find files, anything short of that is a breaking backward >> compatibility. Any change that fixes the ability to find files where before >> it did not find them ( and it should have) if the correct direction. Test >> cases that fail because the scanner is finding files better, when before it >> did not find then and failed ( ie the test expects a failure) is not a >> regression. I believe this would say the B should not be supported as it >> seems to be based on the test failing to pass because the scanner failed to >> find files. When we do this fix and it find files, it works correctly ( ie >> passes) which I would argue is not a bad thing. >> >> >> >> What I was stating below was that the main reason scanner failed for me >> was because bad ordering with subst() caused the scanner to fail ( recusive >> or not) However that is technically a different issue from this >> improvement. I thought might be related, but it is not for the test cases >> in question. My bad .. >> >> >> >> Given what I understand I agree with your suggestion to continue support >> A case and drop B support. >> >> >> >> One question … >> >> Case A: is this correct? >> >> We have A.h -> B.h >> >> The copy action for A.h is to copy as a.h.l >> >> The copy action for B.h is to copy as b.h.l >> >> There is a scanner for *.h >> >> There is no scanner for *.l ?? (I don’t think that matters for this case >> as there no actions with *.h.l files as source files to some other target) >> >> >> >> Actions chain should be like. >> >> >> >> 1) Scons see A.h depends on B.h so it installs b.h as b.h.l, then >> it installs A.h as a.h.l >> >> 2) On rebuild everything is up to date >> >> 3) On modify of b.h scons does 1) set of actions. >> >> >> >> Correct? I think that is efficient and correct. >> >> >> >> Correct. On step three, both B.h and A.h will be reinstalled due to the >> dependency chain even though A.h has not changed. It is correct, but not >> efficient. I believe to fix the efficiency, Install (and copy) should >> behave more like rsync than a plain copy. The install of A.h did not >> happen on step 3 before the scanner improvements. >> >> >> >> I can fix the inefficiency by removing support for Case B. In my opinion >> Case B shouldn't be supported anyway, since I do not think that Use Case >> makes sense for the Install builder. I could possibly see using Copy for >> this Use Case but not Install. The issue with some of the SCons tests are >> that the tests define how the program behaved previously, but they do not >> define how the program was intended to behave. I'm not sure whether Case B >> was supported intentionally or it just happened to work. >> >> >> >> >> >> I should be able to say this in Parts for example as get the same logic >> as the scanner are implicit…. >> >> … >> >> env.CCopyAs([‘a.h.l’,’b.h.l’’],[‘a.h’,’b.h’]) >> >> … >> >> >> >> Since the scanner for the .h files will be run by the CCopy builder in >> Parts for a given file, even though I don’t have a scanner for the source >> or target files in the builder directly. >> >> I should be able add such a test to verify this behavior with the >> implicit scanner on a random builder. Is this correct? >> >> >> >> Thanks Jason >> >> >> >> *From:* Scons-dev [mailto:[email protected]] *On Behalf Of *William >> Blevins >> *Sent:* Tuesday, June 2, 2015 6:00 PM >> *To:* SCons developer list >> *Subject:* Re: [Scons-dev] SCons cross-language support >> >> >> >> Jason, >> >> >> >> If I understand this correctly. I have a similar issue in Parts. I found >> that it was that the scanner did not expand the path correctly via not >> calling subst() before it tried to access the variable. This caused >> failures in correctly finding libs and or header on first pass builds. It >> would also call possible rebuild on second pass or in some cases a false >> rebuild as SCons thought the path changed. >> >> >> >> This isn't (too my understanding) related to the patch in question. >> >> >> >> However I might have missed something in terms of this patch as I did not >> know the install builder called a scanner. ( you did mean the install.py >> tool.. or was this something else?) the install tools as I understand does >> not have a scanner mapped to it. >> >> >> >> All builders call a scanner or at least try to find a scanner to call >> (whether implicitly or explicitly). Sometimes this was simply handled as a >> None case. In order to finish the patch, I need to make a choice (Case A >> or Case B). Neither are 100% backwards compatible with previous behavior, >> but both are reasonable I believe. >> >> >> >> V/R, >> >> William >> >> >> >> >> _______________________________________________ >> Scons-dev mailing list >> [email protected] >> https://pairlist2.pair.net/mailman/listinfo/scons-dev >> >> >> >> >> _______________________________________________ >> Scons-dev mailing list >> [email protected] >> https://pairlist2.pair.net/mailman/listinfo/scons-dev >> >> >> >> >> _______________________________________________ >> Scons-dev mailing list >> [email protected] >> https://pairlist2.pair.net/mailman/listinfo/scons-dev >> >> >
_______________________________________________ Scons-dev mailing list [email protected] https://pairlist2.pair.net/mailman/listinfo/scons-dev
