Build cleanup and checkstyle fixes and rules updates#2901
Build cleanup and checkstyle fixes and rules updates#2901g2vinay merged 13 commits intoAzure:masterfrom
Conversation
|
Can one of the admins verify this patch? |
build-tools/pom.xml
Outdated
| @@ -0,0 +1,16 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
In other repos we are starting to try and switch to using a folder called "eng" to hold files specific engineering system tools and such. I'd suggest calling the folder "eng" instead of "buid-tools".
There was a problem hiding this comment.
Actually it seems as though this is actually project so I'd suggest you still put it under an "eng" folder like "eng\build-tool" assuming you want to call it that. It would probably be best to name it after what it is trying to do.
There was a problem hiding this comment.
My gut feeling is that we should keep this as /build-tools - it's a pretty standard approach in Java to expose build-related tools from the root level. E.g. AWS and GCP. I'm not against having it under a sub-directory, but 1) I wonder what else we might put under there (e.g. in the future presumably we will have a top-level azure-sdk-bom directory, etc, and that wouldn't go under eng) and 2) if 'eng' is really a good, descriptive name for visitors to the repo.
There was a problem hiding this comment.
@weshaggard Can you give your thoughts here please? Thanks.
There was a problem hiding this comment.
I don't have a strong preference here but my goal is to drive consistency across the language repos as much as possible while still allowing for the language conventions where it makes sense. One of my goals is to reduce the clutter in the root of the repo which is why I like having a top level directory that we store anything that is infrastructure/engineering system related into. In general I feel most contributors to the repo generally care more about the source code and less about the engineering system so keeping it scoped mostly in a directory helps eliminate that clutter for a lot of people. For your azure-sdk-bom directory I think that is more of a "source code" type of change, especially if we plan to ship it, so having it in its own directory would make sense. As for the name "eng" we could debate that but it is a convention we have used on my previous teams and it is short name that people who care about what is in there generally learn what it means.
That said I'll leave this call up to you @JonathanGiles if you feel that having the build-tools directory makes more sense in the scope of Java developers please continue with that name, but if you feel this is something that most contributors to the repo won't care about then I'd like to use a common convention and put it under a "eng" sub-dir.
There was a problem hiding this comment.
I'd also add that if there going to be multiple of these tools (e.g. spotcheck, checkstyle, etc) then I personally think having them named after what they are checking and under a sub-directory makes the most sense.
There was a problem hiding this comment.
I'm ok with using eng. I'm work with Vinay on directory naming
There was a problem hiding this comment.
Directories are renamed.
build-tools is -> eng/code-quality-reports
eng/spotbugs-reporting -> eng/spotbugs-aggregate-report.
Build tools is listed as module in pom.client.xml |
eng/spotbugs-reporting/pom.xml
Outdated
| <artifactId>azure-client-sdk-parent</artifactId> | ||
| <groupId>com.azure</groupId> | ||
| <version>1.0.0-SNAPSHOT</version> | ||
| <relativePath>../pom.client.xml</relativePath> |
There was a problem hiding this comment.
This isn't the correct path to the parent pom. One level too low.
| <artifactId>azure-keyvault-webkey</artifactId> | ||
| <version>${azure-keyvault.version}</version> | ||
| </dependency> | ||
| </dependencies> |
There was a problem hiding this comment.
Do we need to specify the dependencies AND the source below?
There was a problem hiding this comment.
Yes,
Build helper plugin dymanically imports the source during build time.
Dependencies ensure that code compiles.
If we just invoke spotbugs:spotbugs it doesn't work, because the aggregated code only gets imported at build time.
As discussed, we will explore ways to optimize this or use open source plugins available.
build-tools/pom.xml
Outdated
| @@ -0,0 +1,16 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
@weshaggard Can you give your thoughts here please? Thanks.
eng/spotbugs-reporting/pom.xml
Outdated
| </goals> | ||
| <configuration> | ||
| <sources> | ||
| <source>..\keyvault\data-plane\azure-keyvault\src\main\java</source> |
There was a problem hiding this comment.
Does this really require that we list all the source directories manually here to run the checks?
There was a problem hiding this comment.
yeah, spotbugs doesn't offer aggregated reports. This way requires manually specifying sources.
There was a problem hiding this comment.
This aggregates all the spotbugs in one report.
There was a problem hiding this comment.
That is going to be a pain to maintain. We should be on the look out for potential ways to configure or generate this list so we don't have to add each project individually.
There was a problem hiding this comment.
yes, agreed,
In next PR we will optimise this or explore other ways to accomplish this by utilizing open source plugins.
weshaggard
left a comment
weshaggard
left a comment
There was a problem hiding this comment.
Looks good. Please squash the commits on merge.
JonathanGiles
added
the
Client
5 participants
Deleted pom.client.build.xml
Updated checkstyle rules.
Updated headers.
Added html report export support for spotbugs.