Skip to content

Spotless updates#331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
karllessard merged 1 commit into from
Jun 1, 2021
Merged

Spotless updates #331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -40,6 +40,32 @@ jobs:
- name: Run lint checks
run: |
mvn compiler:compile -Pdev,jdk11 -B -U -e
check-format:

@karllessardkarllessardJun 1, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do simply like Run link checks above and add the Run format checks as a separate step of quick-build instead of creating a new job?

@rnettrnettJun 1, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I didn't notice those were separate steps. I think it might be helpful to have the output separate though, since the build & tests & lint should be passing before approval, but format should be done after. It's easier (for me at least) to see that w/ fully separate tasks. And I don't know of any way to have multiple check results from the same job.

@karllessardkarllessardJun 1, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if your job fail, you can see pretty easily at which step the failure happened. I really see the format check as the same thing as the lint checks so I think we should replicate how it is done, at least for consistency? Also avoid preparing multiple times these containers makes the quick builder "quicker". Finally, since a mvn install is done in the quick-build prior to the checks, it will allow us to validate the format of the generated files as well (which we want, right?)

@rnettrnettJun 1, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if you open the log and read through. I still think it's easier to see at a glance (i.e. in the emails it sends you, or from the PR list) if it's in a separate job, and they should run in parallel. The difference from the lint checks, imo, is that this is supposed to be done after approval, otherwise I'd make it part of the same job. And that's going to be tricky enough for new contributors (backing out format changes is often hard).

I haven't been formatting the generated code, it's not generated as complaint, so we'd need to essentially do a spotless:apply as part of generation. It would affect the protobuf stuff too. I can, but it didn't seem like something we really needed.

@karllessardkarllessardJun 1, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference from the lint checks, imo, is that this is supposed to be done after approval

That's true! Ok let's try it like this then

if: github.event_name == 'pull_request'
runs-on: ubuntu-latest
container: centos:7
steps:
- name: Checkout repository
uses: actions/checkout@v1
- name: Install environment
run: |
yum -y update
yum -y install centos-release-scl-rh epel-release
yum -y install java-11-openjdk-devel devtoolset-7
echo Downloading Maven
curl -L https://archive.apache.org/dist/maven/maven-3/3.6.3/binaries/apache-maven-3.6.3-bin.tar.gz -o $HOME/apache-maven-3.6.3-bin.tar.gz
tar xzf $HOME/apache-maven-3.6.3-bin.tar.gz -C /opt/
ln -sf /opt/apache-maven-3.6.3/bin/mvn /usr/bin/mvn
- name: Build project
run: |
source scl_source enable devtoolset-7 || true
export JAVA_HOME=$(dirname $(dirname $(readlink $(readlink $(which javac)))))
echo $JAVA_HOME
mvn -version
mvn clean install -Pdev,jdk11 -B -U -e -Dlint.skip=true
- name: Run format checks
run: |
mvn spotless:check -Pdev,jdk11 -B -U -e
prepare:
runs-on: ubuntu-latest
outputs:
Expand Down
29 changes: 5 additions & 24 deletions pom.xml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -230,11 +230,12 @@
</build>
</profile>

<!--
Profile that enables format checks on compilation
Can auto-format using spotless:apply.
-->
<profile>
<id>format</id>
<activation>
<jdk>(,16)</jdk>
</activation>
<id>check-format</id>
<build>
<plugins>
<plugin>
Expand DownExpand Up@@ -340,26 +341,6 @@
<googleJavaFormat/>

<removeUnusedImports/>

<licenseHeader>
<content>
/* Copyright $YEAR The TensorFlow Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
=======================================================================
*/
</content>
</licenseHeader>
</java>
</configuration>
</plugin>
Expand Down