Re: [GBUILD] Can we get rid of recursively invoking custom targets ?
On 20 March 2012 11:17, Michael Stahl wrote: > i think nobody really likes the way custom targets are currently > implemented, and iirc Bjoern even denies being responsible for the design :) So - I re-implemented them. Hopefully it's now better and I did not break too much along the way. With http://cgit.freedesktop.org/libreoffice/core/commit/?id=0a45deba2be4a77db7540bd050b25bd6c26d7513 there is no more room for custom-target packages. Also gbuild_simple.mk is gone. Best, Matus ps: this goes also to the list. Sorry, Michael, for first version accidentally sent only to you. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [GBUILD] Can we get rid of recursively invoking custom targets ?
On Sat, Mar 24, 2012 at 5:00 PM, Matúš Kukan wrote: > On 20 March 2012 21:04, Matúš Kukan wrote: >> On 20 March 2012 11:17, Michael Stahl wrote: >>> but please rename the CDPI variable to packimages_CDPI or expand it >>> (also, CUSTOM_images, CUSTOM_PREFERRED_FALLBACK_[12]) >> >> CDPI was supposed to be custom-target-directory-packimages. >> That's not good because CD would be the same for all custom targets, >> but 3(4) letters should be enough? First two for and third >> (or also fourth) for directory in module. >> It must be enough to avoid collision, we don't have so many modules. > > I decided to use 4 letters and push my patches(*) because i18npool was wrong > and after --disable-dependency-tracking change in gbuild it started to fail. > We still can change the makefiles in case something is wrong or should > be done better. > > Matus > > (*) > http://cgit.freedesktop.org/libreoffice/core/commit/?id=1140c380ad15648def3fc7a71d735a46ed4289d9 > > http://cgit.freedesktop.org/libreoffice/core/commit/?id=15bd35e4f2646ef0bba0cc24d171989c9e3ac6e4 That seems to break my windows build, which now abend on the invocation of saxparser Norbert ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [GBUILD] Can we get rid of recursively invoking custom targets ?
On 20 March 2012 21:04, Matúš Kukan wrote: > On 20 March 2012 11:17, Michael Stahl wrote: >> but please rename the CDPI variable to packimages_CDPI or expand it >> (also, CUSTOM_images, CUSTOM_PREFERRED_FALLBACK_[12]) > > CDPI was supposed to be custom-target-directory-packimages. > That's not good because CD would be the same for all custom targets, > but 3(4) letters should be enough? First two for and third > (or also fourth) for directory in module. > It must be enough to avoid collision, we don't have so many modules. I decided to use 4 letters and push my patches(*) because i18npool was wrong and after --disable-dependency-tracking change in gbuild it started to fail. We still can change the makefiles in case something is wrong or should be done better. Matus (*) http://cgit.freedesktop.org/libreoffice/core/commit/?id=1140c380ad15648def3fc7a71d735a46ed4289d9 http://cgit.freedesktop.org/libreoffice/core/commit/?id=15bd35e4f2646ef0bba0cc24d171989c9e3ac6e4 ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [GBUILD] Can we get rid of recursively invoking custom targets ?
On 20 March 2012 11:17, Michael Stahl wrote: > 1. var namespace collisions: > because make doesn't have namespaces for global variables, it's > possible that variables defined in different modules' custom > makefiles have the same name and thus collide. > this can be avoided by agreeing to a naming convention, such as > prefixing all variables by the name of the module where they are > defined, and reviewing all custom makefiles so that this convention > is met. We have to create custom makefiles, so no problem with reviewing. But is it really such problem ? I mean, we can't use INCLUDE variable of course but if some variable is used in different custom makefile and we are initializing it, there should be no problem, because as make is parsing makefile, it will be properly set. Anyway, we can grep for the name of variable and set it that way, so there is no collision. I would really like to use only short variables for the working directory, because they are used often and I think it is more readable then. > 2. pattern rule collisions: > the pattern rules defined in a lot of custom makefiles also can lead > to collisions, both with other custom makefiles, and with the > pattern rules defined in solenv/gbuild, which is complicated due > to different pattern rule matching in make 3.81 and 3.82. > so the pattern rules in custom makefiles should all have targets of > the form $(WORKDIR)/CustomTarget/$(MODULE)/... and this prefix > should be different across modules and so the problem of getting > these to work in 3.81 and 3.82 should be module local and hence > hopefully manageable. Ah, I did not think about this. So, what are the rules for pattern matching? I thought make 3.81 picks first one and make 3.82 the one with longest matching prefix. oh, well, it does not matter, you are right, (WORKDIR)/CustomTarget/$(MODULE)/ is good since there is nothing colliding. So, no problem. > but please rename the CDPI variable to packimages_CDPI or expand it > (also, CUSTOM_images, CUSTOM_PREFERRED_FALLBACK_[12]) CDPI was supposed to be custom-target-directory-packimages. That's not good because CD would be the same for all custom targets, but 3(4) letters should be enough? First two for and third (or also fourth) for directory in module. It must be enough to avoid collision, we don't have so many modules. Also, there is no chance anyone would want to use such terrible name as CUSTOM_images and I think he is responsible for the name (it's easy to grep) and moreover if there would be collision, it should work anyway, or not ? Thanks for reviewing, Matus ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [GBUILD] Can we get rid of recursively invoking custom targets ?
On 16/03/12 15:47, Matúš Kukan wrote: > Hi, > > I'd like to propose removing gbuild_simple.mk over time. > Surely there will be more custom targets as gbuild is growing and I > can't see why we would need gbuild_simple. > Instead we can write rules to Package_foo.mk makefiles or if that > would be problem (because they will be ugly) > We can start using CutomTarget_foo.mk which will be supposed to be ugly. > > The reasons are: > 1, Sometimes we need to parse custom target's Makefile each time as in > packimages case. > I had to use: > $(eval $(call gb_CustomTarget_add_outdir_dependencies,packimages/pack,\ > $(gb_Helper_PHONY) \ > )) > to achieve that. > > 2, It is enough then to write dependencies only once. > > 3, No need to maintain gbuild_simple. > > 4, No recursive make running. i think nobody really likes the way custom targets are currently implemented, and iirc Bjoern even denies being responsible for the design :) having to write dependencies twice so the recursive make invocation works is really annoying and error-prone. there are some potential problems with having the custom stuff in the same make process, but i think these are solvable: 1. var namespace collisions: because make doesn't have namespaces for global variables, it's possible that variables defined in different modules' custom makefiles have the same name and thus collide. this can be avoided by agreeing to a naming convention, such as prefixing all variables by the name of the module where they are defined, and reviewing all custom makefiles so that this convention is met. 2. pattern rule collisions: the pattern rules defined in a lot of custom makefiles also can lead to collisions, both with other custom makefiles, and with the pattern rules defined in solenv/gbuild, which is complicated due to different pattern rule matching in make 3.81 and 3.82. so the pattern rules in custom makefiles should all have targets of the form $(WORKDIR)/CustomTarget/$(MODULE)/... and this prefix should be different across modules and so the problem of getting these to work in 3.81 and 3.82 should be module local and hence hopefully manageable. [for non-custom makefiles both of these problems are avoided by designing gbuild so that variables and pattern rules are only defined in solenv/gbuild, not in user makefiles.] > I am sending two patches for packimages. > Maybe we should use the second one and keep Package_foo.mk pretty ? > One small disadvantage is that we need to depend on directory. > > It allows as to use compatible mode and keep old way of processing > custom targets until they are replaced. > There are 31 custom targets now, so that should not be a big problem. > I can do it over time or maybe we could even write an easy hack for that. > The important thing is to write new custom targets in > CustomTarget_foo.mk makefile. > > What do you think ? Am I missing something ? so overall i think your proposal is a good direction to go. your second patch (with the CustomTarget_foo.mk) looks better to me, as having the ugly custom stuff easily discoverable is a good idea; but please rename the CDPI variable to packimages_CDPI or expand it (also, CUSTOM_images, CUSTOM_PREFERRED_FALLBACK_[12]) ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
