do not run linters on repeated nightly

[mikaelarguedas]

No description provided.

[dirk-thomas]

The label is linter (e.g. see https://github.com/ament/ament_lint/blob/27b56bf60ff1b7bc039cd8403b28373a50449a3f/ament_cmake_pep8/cmake/ament_pep8.cmake#L57).

[mikaelarguedas]

my bad, works locally with lint for some reason. Updated in 78211f1

[dirk-thomas]

How did you test this locally? Currently the patch passes -LE to ament but it needs to pass it to ctest.

[dhood]

I think this needs to be changed to --retest-until-fail 20 --ctest-args -LE linter -- because it's not an ament-specific test argument

[mikaelarguedas]

Sorry local aliases took care the ctest-args for me. Will update it, test it on the farm and resubmitted for review once it's actually ready

[dhood]

I think what's happening here is that the --ctest-args (greedy) are being nested within another greedy argument --ament-test-args that gets the delimeter appended here. Really these two lists should have different variables.

As you mentioned offline, this will work as-is for now, so we can apply it to the farm, but let's not merge it until it's implemented in a way that's more future-proof

[mikaelarguedas]

Tested this on the fastest nightly (windows) it looks like we can divide the time of the repeated nightlies by 2 (tested only on windows the fastest one):
before: http://ci.ros2.org/view/nightly/job/nightly_win_rep/468
after: http://ci.ros2.org/view/nightly/job/nightly_win_rep/470
agreed with #9 (comment) we can configure the nightlies manually for now and iterate on both the patch and the argument parser/delimiter management later on

[mikaelarguedas]

I compared the list of tests with and without this change. As expected the diff only shows the linter tests (test lists for reference)

If there is no objections I will reconfigure manually all the repeated jobs and merge ros2/common_interfaces#23 tonight

[wjwwood]

@mikaelarguedas when you say manually reconfiguring them, you mean through the Jenkins web interface? Or do you mean: merge this and apply the changes using the script in this repository?

[mikaelarguedas]

@wjwwood I mean through the jenkins web interface for the time being. And not merge this until we have the adequate fixes to the argument parsing mentionned in #9 (comment)

[wjwwood]

Ah, sorry misread that comment. 👍

[dirk-thomas]

I would highly recommend to not let the current master diverge from what is being deployed on Jenkins. Simply because it makes deploying any kind of changes much more effort (with a high chance of loosing previously made changes). You can make whatever changes you want to apply manually in a separate PR and that can be merged and then deployed through the script.

[mikaelarguedas]

The changes I made manually are exactly the same as the ones in this PR. You advise to merge this one and deploy the change. Then open a follow up PR to deal with the nested argument parsing + various patches, is that right ?

[dirk-thomas]

I though this one shouldn't be merged since it already deals with it but incompletely. Yes, then this can be merged and deployed and the remaining problem ticketed.

I recently worked with argparse in a different project and I am not sure if the manual extraction of these greedy argument (like --cmake-args a b c --) is a good idea. I think there is a decent way to achieve that without the need for custom parsing. The nested arguments just need to be quoted and prefixed with a space when they start with a dash. Maybe that would be a better approach...

[dhood]

I didn't realise that ament_test_args was a string containing everything that gets passed to ament test, I was thinking there was an --ament-test-args argument for ament like there is --ctest-args. That's why I mentioned that --ctest-args should have its own variable. Now that I understand better, I take back what I said, and think this PR doesn't make the system any more fragile than it was before (vulnerable to --s in user-passed test arguments).

This issue that needs to be ticketed, as I understand it, is to better handle multiple -- delimiters. Either by only appending the extra one here, for example, if it's actually necessary, or by escaping them with quotes+spaces. Does that match your understanding @mikaelarguedas ?

[mikaelarguedas]

Yes that's what I understood too, if we decide not to make the tool robust to double delimiters as stated in ament/ament_tools#111 I'd lean toward just making sure the command doesn't end with -- before appending them. It's going to be a trivial change and will get around the current limitation

[mikaelarguedas]

the fix suggested in #9 (comment) to append the delimiters at the end of the line only if needed has been implemented in #15.
CI jobs using it: Linux Windows