updating make test-math-dependencies; fixing warnings #1107

syclik · 2019-01-23T05:20:57Z

Summary

This PR does 3 things.

It fixes the math dependencies errors that are currently in develop. These errors are in stan/math/prim/scal/meta/StdVectorBuilder.hpp where it pulls an arr header into scal. This was fixed by splitting the template metaprogram's base class into prim (and adding a test for that).
Updating the make test-math-dependencies so that it:
1. returns a non-zero exit code if there are warnings
2. ignores references to std::vector and Eigen::Matrix within comments.
Updates the Jenkinsfile so that it uses the new warnings next generation plugin for parsing some of the warnings. The parser for math-dependencies is still using the old warnings plugin (that's been deprecated) because the mechanism for adding groovy scripts to parse is no longer supported except through the web interface. I figured it's just as well that we leave it as-is.

Tests

The code for contains_std_vector.hpp has been tested with a new unit test in prim. It just makes sure the base case is correct.

For the makefile, I've been running it by hand. When there are no errors, it exits with code 0, if there are errors, it exits with a non-zero return code. I've observed the Jenkins build failing due to warnings and that's our intended effect.

Side Effects

None.

Checklist

Math issue Jenkins: turn test-math-dependencies warnings into PR failures. #1078
Copyright holder:

Generable

The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
  - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
  - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

the basic tests are passing
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

seantalts · 2019-01-24T21:26:21Z

❤️ Thanks! I wonder if now we can remove the script { node { part - that was necessary with the old, non-pipeline friendly warnings plugin to let it work on stages that executed on other nodes.

seantalts · 2019-01-24T21:27:15Z

Jenkinsfile

@@ -127,8 +127,7 @@ pipeline {
            }
            post {
                always {
-                    warnings consoleParsers: [[parserName: 'CppLint']], canRunOnFailed: true
-                    warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true


Looks like you might have lost the math-dependencies tool?

There are two things going on:

I don't know where the math-dependencies tool is? Meaning the parser for Jenkins. The make test-math-dependencies is just some fancy scripting.

The format of make test-math-dependencies adheres to cpplint, so if we can parse cpplint output, we should just be able to use that. I think when I had originally set it up, that's what I did in Jenkins (just used the CppLint parser).

Is there a different place I should be looking for the math-dependencies parser? I wonder if I missed it cause it's in a different repo.

seantalts · 2019-01-24T23:46:59Z

It's a custom parser set up somewhere in Jenkins master config (on phone ATM or I'd try to find it). That said maybe we don't need it? I didn't add it.

On Thu, Jan 24, 2019 at 16:41 Daniel Lee ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Jenkinsfile <#1107 (comment)>: > @@ -127,8 +127,7 @@ pipeline { } post { always { - warnings consoleParsers: [[parserName: 'CppLint']], canRunOnFailed: true - warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true There are two things going on: 1. I don't know where the math-dependencies tool is? Meaning the parser for Jenkins. The make test-math-dependencies is just some fancy scripting. 2. The format of make test-math-dependencies adheres to cpplint, so if we can parse cpplint output, we should just be able to use that. I think when I had originally set it up, that's what I did in Jenkins (just used the CppLint parser). Is there a different place I should be looking for the math-dependencies parser? I wonder if I missed it cause it's in a different repo. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1107 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxJ7GXwbdXLErU4nful4vivh7F3N-FJks5vGihqgaJpZM4aODuI> .

-- 📲

syclik · 2019-01-25T19:47:55Z

It's a custom parser set up somewhere in Jenkins master config (on phone ATM or I'd try to find it). That said maybe we don't need it? I didn't add it.

Cool. Less is better than more. Let's see if we can get it done without a custom parser.

…ies errors

…encies

…gs/RELEASE_500/final)

syclik · 2019-02-01T03:17:01Z

@seantalts, I updated the PR. Once it passes tests, it'll be ready for review.

into feature/issue-1078-ci

seantalts · 2019-02-01T03:27:17Z

Jenkinsfile

-                    warnings consoleParsers: [[parserName: 'CppLint']], canRunOnFailed: true
-                    warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true
+                    recordIssues enabledForFailure: true, tool: cppLint()
+		    warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true


Did you try recordIssues enabledForFailure: true, tool: math-dependencies()? Would be good to get rid of the old warnings plugin completely as I think it's going away soon.

I had tried before. Just to make sure, I tried again. This is the doc that makes me think that we can't do it that way: https://github.com/jenkinsci/warnings-ng-plugin/blob/master/doc/Documentation.md#creating-a-new-tool-in-the-user-interface

It says that you can have a Groovy parser, it just needs to be configured using the web interface. I also tried those instructions, but to no avail. (I'm sure it can't find the groovy script in the right place.)

Ah gotcha. Let me take a quick crack at it.

Sorry, I wasn't clear. I followed the instructions to run the Groovy script, but didn't add it via the web interface. Hopefully it does work!

I added it via the web interface and pushed the change to the Jenkinsfile. Will it show up as working in this build or do we have to trigger some tests for it?

(fixing Jenkinsfile syntax - it changed a lot and apparently you can't scan the console log anymore, so we need to direct to a file).

syclik · 2019-02-01T21:35:00Z

You'll need to add something to trigger it. I've been including a rev header into stan/math/prim/scale.hpp as a single commit so I can back it out easily.

…

On Fri, Feb 1, 2019 at 4:18 PM seantalts ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Jenkinsfile <#1107 (comment)>: > @@ -127,8 +127,8 @@ pipeline { } post { always { - warnings consoleParsers: [[parserName: 'CppLint']], canRunOnFailed: true - warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true + recordIssues enabledForFailure: true, tool: cppLint() + warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true I added it via the web interface and pushed the change to the Jenkinsfile. Will it show up as working in this build or do we have to trigger some tests for it? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1107 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F1ioLz8fdY8uIw3YzO8T7OzgqTeAks5vJK8wgaJpZM4aODuI> .

seantalts · 2019-02-01T21:42:14Z

Ooh, pretty: http://d1m1s1b1.stat.columbia.edu:8080/job/Math%20Pipeline/job/PR-1107/26/mathDependencies/

syclik · 2019-02-01T21:45:45Z

Nice!

seantalts · 2019-02-01T22:13:50Z

Okay, I think I'm done messing with the branch. I think the math-dependencies are added in a decent way (though it's a little ugly to get output & exit status while logging to a file so the parser can read it, which I read was necessary but didn't experiment with).

syclik · 2019-02-02T04:43:11Z

Jenkinsfile

-                    warnings consoleParsers: [[parserName: 'math-dependencies']], canRunOnFailed: true
+                    recordIssues enabledForFailure: true, tools:
+                        [cppLint(),
+                         groovyScript(parserId: 'mathDependencies', pattern: '**/dependencies.log')]


syclik · 2019-02-02T04:44:31Z

Thanks for figuring that out! It's passed tests. When you get a chance, mind reviewing and merging if you think it's ready?

updating Jenkinsfile warnings plugin

136010f

syclik force-pushed the feature/issue-1078-ci branch from 8369695 to 136010f Compare January 23, 2019 05:40

seantalts reviewed Jan 24, 2019

View reviewed changes

syclik force-pushed the feature/issue-1078-ci branch 2 times, most recently from 58a8f06 to 016a7ae Compare January 31, 2019 17:12

forcing make to exit with an error code when there are math-dependenc…

15bf000

…ies errors

syclik force-pushed the feature/issue-1078-ci branch from 016a7ae to 15bf000 Compare January 31, 2019 17:17

syclik added 8 commits January 31, 2019 12:26

trying to get the test-math-dependencies to show up in Jenkins

cf2d5d4

removing extraneous echo

c045a86

removing redundant line

e2d7345

splitting contains_std_vector to scal and arr to deal with the depend…

e824492

…encies

adding test for contains_std_vector

05f07da

trying to add back in the math-dependencies parser

69b0ffd

figured out how to ignore comments in test-math-dependencies

7240915

removing whitespace

ec39f30

syclik force-pushed the feature/issue-1078-ci branch from 4dcf880 to 69b0ffd Compare February 1, 2019 03:01

yashikno and others added 3 commits February 1, 2019 03:02

Merge commit '6bb33cfab074d1bdba1888c80458b278a435564d' into HEAD

256c396

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

51f3783

…gs/RELEASE_500/final)

updating stan/math/prim/scal.hpp

f620cf4

syclik changed the title ~~[WIP] updating Jenkinsfile warnings plugin~~ updating make test-math-dependencies; fixing warnings Feb 1, 2019

Merge branch 'feature/issue-1078-ci' of https://github.com/stan-dev/math

0d37b4d

into feature/issue-1078-ci

seantalts reviewed Feb 1, 2019

View reviewed changes

syclik force-pushed the feature/issue-1078-ci branch from 50b8399 to 0d37b4d Compare February 1, 2019 19:37

seantalts force-pushed the feature/issue-1078-ci branch from a2ca0c2 to 659852f Compare February 1, 2019 21:23

Try new mathDependencies Groovy script with new Warnings plugin

6864e74

seantalts force-pushed the feature/issue-1078-ci branch from 659852f to 6864e74 Compare February 1, 2019 21:30

Test commit for mathDependencies

84806e2

seantalts force-pushed the feature/issue-1078-ci branch from 3232ec1 to 037c938 Compare February 1, 2019 22:01

Tee so we get some output on the blue ocean page.

8ad02f2

seantalts force-pushed the feature/issue-1078-ci branch from 037c938 to 8ad02f2 Compare February 1, 2019 22:06

Remove error used for testing.

ad3c454

syclik commented Feb 2, 2019

View reviewed changes

seantalts approved these changes Feb 2, 2019

View reviewed changes

seantalts merged commit 538b583 into develop Feb 2, 2019

syclik mentioned this pull request Feb 19, 2019

Some required headers are not included at is_vector, is_vector_like and check_not_nan #1106

Closed

syclik deleted the feature/issue-1078-ci branch February 19, 2019 02:58

syclik mentioned this pull request Feb 23, 2019

Jenkins: turn test-math-dependencies warnings into PR failures. #1078

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updating make test-math-dependencies; fixing warnings #1107

updating make test-math-dependencies; fixing warnings #1107

syclik commented Jan 23, 2019 •

edited

Loading

seantalts commented Jan 24, 2019

seantalts Jan 24, 2019

syclik Jan 24, 2019

seantalts commented Jan 24, 2019 via email

syclik commented Jan 25, 2019

syclik commented Feb 1, 2019

seantalts Feb 1, 2019

syclik Feb 1, 2019

seantalts Feb 1, 2019

syclik Feb 1, 2019

seantalts Feb 1, 2019 •

edited

Loading

syclik commented Feb 1, 2019 via email

seantalts commented Feb 1, 2019

syclik commented Feb 1, 2019

seantalts commented Feb 1, 2019

syclik Feb 2, 2019

syclik commented Feb 2, 2019

updating make test-math-dependencies; fixing warnings #1107

updating make test-math-dependencies; fixing warnings #1107

Conversation

syclik commented Jan 23, 2019 • edited Loading

Summary

Tests

Side Effects

Checklist

seantalts commented Jan 24, 2019

seantalts Jan 24, 2019

Choose a reason for hiding this comment

syclik Jan 24, 2019

Choose a reason for hiding this comment

seantalts commented Jan 24, 2019 via email

syclik commented Jan 25, 2019

syclik commented Feb 1, 2019

seantalts Feb 1, 2019

Choose a reason for hiding this comment

syclik Feb 1, 2019

Choose a reason for hiding this comment

seantalts Feb 1, 2019

Choose a reason for hiding this comment

syclik Feb 1, 2019

Choose a reason for hiding this comment

seantalts Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

syclik commented Feb 1, 2019 via email

seantalts commented Feb 1, 2019

syclik commented Feb 1, 2019

seantalts commented Feb 1, 2019

syclik Feb 2, 2019

Choose a reason for hiding this comment

syclik commented Feb 2, 2019

syclik commented Jan 23, 2019 •

edited

Loading

seantalts Feb 1, 2019 •

edited

Loading