[opencms-dev] How to code-review

[Alberto Gallardo]

Hi Tobias,

under which circumstances do you end up with wrong binaries?
>

At least, there are two details that can lead to unexpected results:
- The inclusion of mavenLocal() as repository: if we build and install any
of the maven dependencies locally, these binaries will be used for the
gradle build without warnings. We probably want gradle to only build
against official artifacts, and not against possible remainders in our
.m2/repository.
- 'dist_*' tasks: they do not depend on the corresponding 'jar_*' task. If
the binaries are on their paths, Gradle builds these zips without
rebuilding them, even if it were necessary.


> Also, the test cases do take time to execute, this is not related to any
> possible inefficiencies of the build script. You don't need to run the test
> cases to build the binaries.
>

That's very true. And I haven't found any room for performance improvements
yet. (I still wanted to take a look into parallel executions, but this has
no priority.)


Other tasks that take a lot of time are generating GWT resources, this is a
> general issue with GWT and not related to the build script.
>
> And if you build the JavaDoc jars this will take a lot of time as well,
> but this is related to the amount of source code and not to the build
> script.
>

The point is that the build script is missing some javadoc files (see
http://www.opencms-wiki.org/wiki/Talk:OpenCms_Documentation/Server_installation/OpenCms_shell).
And is very hard to notice, because it issues a lot of warnings.

So, if you think there is a way to significantly safe time during build,
> please point out, what you think should be changed.
>

I don't have yet any 'silver bullet' to get 'significant' time savings, but
I still have some hope:

- With gradle 4.0, the build cache is production ready, and I was hoping to
be able to use it. I have to further investigate this. (Again, it'd be
great to use the gradle wrapper)
- To improve incremental builds, the 'enhance' task should also be
reviewed. Apparently, it gets always executed (the 'outputs.upToDateSpec'
trick doesn't seem to work). And it changes also the classes, so it
invalidates caches, and after invoking it, the compileJava will be
triggered again. This could be a problem of the gradle version used: that's
the reason I'd like to have the wrapper, but I don't really know until I
test a bit more.

My biggest issues though are related to the bad IDE support of some
functionalities that are in theory natively supported. For example, the
org.opencms.* source sets define different java.includes with the same base
moduleSrcFolder. IDEA doesn't like this, and I'm not sure if it could have
side effects. IDEA also has native support for vaadin, gwt and some other
extended features. But not with the current script, or not without
configuring the project (I don't know how). And for that, I have to first
analyse the script and find how to tame the beast.

The gradle script is pretty big and hard to grasp. It has taken me quite a
while to understand it (and I'm still on it). And while on reviewing it, I
though 'why not try to improve it?'. That's why I also tried to deduplicate
code and get rid of the warnings.

But as you pointed out, all of this could be seen as 'cosmetic changes'. I
didn't want to distract you from other tasks until I myself am satisfied
with refactorings and changes.



Regards,


Alberto
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://webmail.opencms.org/pipermail/opencms-dev/attachments/20170620/0d78f0a8/attachment.htm>