[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #20596 [Metrics]: streamline build.xml and metrics_checkstyle.xml throughout all java projects

#20596: streamline build.xml and metrics_checkstyle.xml throughout all java
projects
-------------------------+--------------------------------
 Reporter:  iwakeh       |          Owner:  iwakeh
     Type:  enhancement  |         Status:  needs_revision
 Priority:  Medium       |      Milestone:
Component:  Metrics      |        Version:
 Severity:  Normal       |     Resolution:
 Keywords:               |  Actual Points:
Parent ID:               |         Points:
 Reviewer:               |        Sponsor:
-------------------------+--------------------------------
Changes (by karsten):

 * status:  needs_review => needs_revision


Comment:

 Whee, quite some code to review here.  Sure, a lot of code has just moved
 around, but that only makes it trickier to spot the little differences.  I
 finished a first (tool-supported) read of the changes but didn't finish
 trying it all out.  But hopefully this initial feedback is useful anyway:

 https://gitweb.torproject.org/metrics-
 base.git/commit/?id=7a01d68f813cb5ae2904e73dbc81999ce0622eca

  - The two properties `"libs"` and `"source-and-target-java-version"` are
 listed twice.

  - Speaking of `"source-and-target-java-version"`, we should probably keep
 that property project-specific.  Otherwise we'll have to raise the Java
 version for all projects at once, including metrics-lib and projects
 depending on metrics-lib.

  - The Checkstyle configuration file `java/metrics_checks.xml` does not
 contain modules `SuppressWarningsFilter` and `SuppressWarningsHolder` that
 we added in metrics-lib's version nor module `UnusedImports` in Onionoo's
 version.  It also still defines a maximum line length of 100 rather than
 80.  Let's combine the latest settings from all three code bases here.

  - This is certainly a minor issue, but the usage instructions in the
 `usage` target should all use the same capitalization and sentence
 structure, like "'name' does XY", not "'name' Does XY" or "'name' XY".

  - It's already time to update the copyright year in the `docs` target!
 Happy 2017!

 https://trac.torproject.org/projects/tor/attachment/ticket/20596/0001
 -Implements-part-of-task-20596.patch

  - Typo in `java/build.xml.template`: "jarpatterprop" is missing an "n".

  - By the way, I already pushed this commit to facilitate testing.  We can
 fix things in subsequent commits.

 https://gitweb.torproject.org/user/iwakeh/onionoo.git/commit/?h=task-20596-submod&id=0442032f24c8764d6c055ed3b9f4e047fbaa886e

  - Why did you change the `"jetty.version"` property to `""` rather than
 `"-8.1.16.v20140903"`?

  - The new generic `tar` target does not include the `DESIGN` file
 anymore.  I'd argue that we should remove that file anyway, but if you
 want to keep it, we'll have to include it in the release tarball, too.

  - The script file `src/main/resources/bootstrap-development.sh` should be
 checked in with the executable flag.

  - This commit or a subsequent commit should remove
 `src/test/resources/metrics_checks.xml` which is now obsolete.

 https://gitweb.torproject.org/user/iwakeh/metrics-
 lib.git/commit/?h=task-20596-submod&id=60d20b85c4dac3d9d0905a185b7bc928736f7fcf

  - Which tests did not pass prior to this commit?  They run through just
 fine here.

  - It shouldn't be necessary to initialize attributes with `null` or `0`,
 which is the default anyway.  All new no-args constructors should be empty
 (except for a comment saying that they're empty on purpose).

 https://gitweb.torproject.org/user/iwakeh/metrics-
 lib.git/commit/?h=task-20596-submod&id=946c403c357661a2290b424ecb1a1c70d70c1334

  - The script file `src/main/resources/bootstrap-development.sh` should be
 checked in with the executable flag.

  - This commit or a subsequent commit should remove
 `src/test/resources/metrics_checks.xml` which is now obsolete.

 https://gitweb.torproject.org/user/iwakeh/metrics-
 lib.git/commit/?h=task-20596-submod&id=7903d6a55504801dd03c75867ce79ff8f3799f11

  - Looks good!

 https://gitweb.torproject.org/user/iwakeh/collector.git/commit/?h=task-20596-submod&id=277be1b5802f8a7408fad27748ca00dab802d54f

  - The script file `src/main/resources/bootstrap-development.sh` should be
 checked in with the executable flag.

  - This commit or a subsequent commit should remove
 `src/test/resources/junittest.policy` and
 `src/test/resources/metrics_checks.xml` which are now obsolete.

 I guess I'll wait for the revision before trying out these changes
 altogether.  My plan was to create release tarballs and compare them with
 the latest releases.

 Thanks again for making build processes better!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20596#comment:29>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs