Skip to content

Conversation

NathanJPhillips
Copy link
Contributor

This PR makes the linter play nicely with our new sub-module layout.

@tautschnig
Copy link
Collaborator

Looks good to me, but I'd suggest @thk123 and @jgwilson42 to be the reviewers of this one.

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Don't understand the break but otherwise looks good.

Copy link
Contributor

Choose a reason for hiding this comment

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

How come this break is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we want to find the git repository immediately containing the working directory, not the one closest to root. Otherwise this doesn't match up with the folder used by the git diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK that makes sense.

@peterschrammel
Copy link
Member

@forejtv, please have a look why the Linter is unhappy with the new "ignoring" output.

@NathanJPhillips
Copy link
Contributor Author

@tautschnig can we get this merged now?

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@forejtv
Copy link
Contributor

forejtv commented Mar 20, 2017

With @NathanJPhillips's help, I have a fix for the linter:

Change https://github.com/diffblue/cbmc/blob/master/scripts/run_lint.sh#L65 to

result=`$script_folder/cpplint.py $file 2>&1 | $script_folder/filter_lint_by_diff.py $diff_file $absolute_repository_root | grep -v "Ignoring .*; not a valid file name" | cat`

@peterschrammel
Copy link
Member

@NathanJPhillips, can you apply this fix please? Then we can merge

@NathanJPhillips NathanJPhillips changed the title Bugfix/lint work across repos Bugfix/lint infrastructure improvements Mar 21, 2017
@NathanJPhillips NathanJPhillips force-pushed the bugfix/lint-work-across-repos branch from e4cf395 to 2175b3d Compare March 21, 2017 13:41
Made linter use paths relative to current submodule, not the lowest containing repository
Use the path of the containing repository of the current path, not the one the script is downloaded into
@NathanJPhillips NathanJPhillips force-pushed the bugfix/lint-work-across-repos branch from 2175b3d to 36dee83 Compare March 21, 2017 14:10
@NathanJPhillips
Copy link
Contributor Author

Turns out I was an idiot with one of my previous changes here. I've undone that and made it all good now.

Copy link
Contributor

Choose a reason for hiding this comment

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

What was the purpose of # before? Are you sure you can remove that check?

@forejtv forejtv merged commit d4d44db into diffblue:master Mar 21, 2017
@NathanJPhillips NathanJPhillips deleted the bugfix/lint-work-across-repos branch March 21, 2017 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants