Add Nohttp Checks

[rwinch]

No description provided.

[snicoll]

Thanks for the PR @rwinch

Looks good to me, I've added a few comments. Did you have a target branch in mind for this? Looks like 5.1.x would be a good candidate.

[rwinch]

Did you have a target branch in mind for this? Looks like 5.1.x would be a good candidate.

@snicoll Thanks for the comments. I was not sure where the Framework team wanted this merged. It appears that @jhoeller is ok with the general changes going into 5.1.x except for the nohttp being added to the build. I'll let him verify what his thoughts are.

As per the discussion on Slack, I pushed changes to make it so that BeanDefinitionParserDelegate now uses http://www.springframework/schema/ so it is automatically ignored by nohttp. This means there is no need for suppressions.xml at all anymore.

[jhoeller]

Indeed, the documentation changes in particular could easily go into 5.1.x.
The license URL updates in the new source files are clearly master only, as are the tool-related changes.

[rwinch]

@snicoll @jhoeller Thanks for all the feedback. Is there anything else you need from me on this?

[snicoll]

@rwinch a build based on that branch does not work for me. Is that to be expected?

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleNohttp'.
> Checkstyle rule violations were found. See the report at: file:///Users/snicoll/workspace/pivotal/spring-framework/build/reports/checkstyle/nohttp.html
  Checkstyle files with violations: 1024
  Checkstyle violations by severity: [error:6153]

[snicoll]

Thanks for the PR, I've added a few comments. I must share I am bit confused as when to add https and when http is ok in example URLs. I wonder how the tool is supposed to catch that.

Also, 18 commits sounds a bit invasive for such a change. I had started to polish it to 3: one to add the plugin to the build, one to change the header to https and one with all the rest (with the note that one commit should be a separate PR IMO).

Would you mind rebase and squash the URLs changes in one commit?

[jhoeller]

@rwinch As Stephane suggested, can we decompose this PR a bit? Separating the nohttp tool bit from the doc changes seems worthwhile indeed. Also, it's not entirely clear to me either which example URLs we should really be using now.

[jhoeller]

FWIW, I'll make the license headers in those two new test sources consistent with the rest in a separate revision. That's really a plain oversight even aside from the nohttp tool, in contrast to the headers in the gradlew scripts which are generated... So those remaining changes are really primarily motivated by the tool, and we need to take into account that those gradlew scripts get regenerated on a Gradle upgrade.

[rwinch]

@rwinch a build based on that branch does not work for me. Is that to be expected?

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleNohttp'.
> Checkstyle rule violations were found. See the report at: file:///Users/snicoll/workspace/pivotal/spring-framework/build/reports/checkstyle/nohttp.html
  Checkstyle files with violations: 1024
  Checkstyle violations by severity: [error:6153]

It does not fail for me. Can you post more details (i.e. files and the failures)? Perhaps you have stale files from the build or IDE still? Did you try a fresh clone or using git clean -dfxn (remove the n and run again if you are fine with the files being removed).

Thanks for the PR, I've added a few comments. I must share I am bit confused as when to add https and when http is ok in example URLs. I wonder how the tool is supposed to catch that.

See https://github.com/spring-io/nohttp/tree/master/nohttp#thought-process

Also, 18 commits sounds a bit invasive for such a change. I had started to polish it to 3: one to add the plugin to the build, one to change the header to https and one with all the rest (with the note that one commit should be a separate PR IMO).

Would you mind rebase and squash the URLs changes in one commit?

No problem. Done.

[rwinch]

It seems there might be differing opinions on how to split up the commits. I updated to switch to how @snicoll suggested the commits be split up (mostly because it was the first comment I saw). If you prefer, please feel free to split up the commits in another way.

[sbrannen]

I reviewed the changes within the GitHub UI, and I didn't notice anything that seemed out of place.

[rwinch]

@snicoll The nohttp task isn't really intended for anything other that fixing an existing codebase. At the moment, it doesn't use settings from the nohttp extension. This means that everything is included by default. To use it, follow the examples in the documentation.

Please run checkstyleNohttp to verify the build. For me this runs in 3 seconds and passes. Note that the plugin adds checkstyleNohttp to a dependency to check task, so you running check executes checkstyleNohttp.

[snicoll]

Yes, I've edited my comment in the meantime. It does not run in 3s for me though. What about the out directories? Excluding them would also allow duplicate checks.

[rwinch]

Yes, I've edited my comment in the meantime.

Hmm...not sure I understand this. I don't see an edit since I commented?

It does not run in 3s for me though.

How long does it take?

What about the out directories? Excluding them would also allow duplicate checks.

You could add an exclusion on them in nohttp configuration if you want to. I'm not sure how much this is going to gain since *.class files are already excluded.

[snicoll]

In my edit 5h ago, I wrote the following

I got confused initially as there is a separate task for checkstyle that took 20s.

[rwinch]

Ok thanks that clarifies the edit and the amount of time it is taking. I'm still wondering:

You could add an exclusion on them in nohttp configuration if you want to. I'm not sure how much this is going to gain since *.class files are already excluded.

[snicoll]

@rwinch yeah probably my use case was odd. I hadn't rebuild your PR in my IDE so it blew up but I guess once we're good it's not really necessary. The only use case I see is someone making a mistake in an xml file, fixing it, then the build would fail again until a build is done at the IDE level. I still think excluding those directories would be better (especially as it represents a duplication)

[rwinch]

I pushed an update to my branch that:

• merges your changes for rebasing off master
• excludes the out directory of each project

[snicoll]

Running the build breaks for me as it's looking at spring-framework/spring-websocket/bin/spring-websocket.log. This may be an old artifact in my local repo but ignoring .log file looks sensible to me.

[snicoll]

@rwinch did you test this branch against a fresh checkout or something (i.e. without building the framework first). I thought I had some outdated artifacts but it looks like the exclusions are not properly applied.

Here is a gist that shows the error I currently have: https://gist.github.com/snicoll/ac6b5ba8c469bd79a22183704b5df852

I am happy to revisit those and polish the PR but I'd like to first understand if I am missing something and if it's ok to exclude those resources.

[rwinch]

The build works for me even though I have built other branches.

It seems like the failures you are getting are due to the build directory of modules that no longer exist in source control. For example, spring-instrument-tomcat and spring-struts do not exist in master. We should only exclude build directories of actual projects. You should make sure to clean that up before checking.

[snicoll]

Sorry @rwinch I was focused on the spring-websocket/bin and I thought that one was legit. I should have reviewed the list more carefully.

It's now merged, I also upgraded to 0.0.3.RELEASE. Thanks!

[sbrannen]

The updates to whitelist.lines somehow got lost in the merge, but I added them in 5e9a22d.

So hopefully that takes care of everything for this PR.

[snicoll]

Ooops, for some reason I had a stale version of the PR. Thanks for noticing Sam!