При чтении таких обзоров возникает ощущение, что речь идёт про волшебный эликсир: нажми на кнопку, и вот он — список дефектов перед глазами. Кажется, что по мере совершенствования анализаторов, багов автоматически будет находиться всё больше и больше, а продукты, просканированные этими роботами, будут становиться всё лучше и лучше, без каких-либо усилий с нашей стороны.
Но волшебных эликсиров не бывает. Я хотел бы поговорить о том, о чём обычно не говорят в постах вида «вот какие штуки может найти наш робот»: чего не могут анализаторы, в чём их реальная роль и место в процессе поставки софта, и как внедрять их правильно.
Храповик (источник: википедия).
Чего никогда не смогут статические анализаторы
Что такое, с практической точки зрения, анализ исходного кода? Мы подаём на вход некоторые исходники, и на выходе за короткое время (гораздо более короткое, чем прогон тестов) получаем некоторые сведения о нашей системе. Принципиальное и математически непреодолимое ограничение состоит в том, что получить мы таким образом можем лишь довольно узкий класс сведений.
Самый знаменитый пример задачи, не решаемой при помощи статического анализа — проблема останова: это теорема, которая доказывает, что невозможно разработать общий алгоритм, который бы по исходному коду программы определял, зациклится она или завершится за конечное время. Расширением данной теоремы является теорема Райса, утверждающая, для любого нетривиального свойства вычислимых функций определение того, вычисляет ли произвольная программа функцию с таким свойством, является алгоритмически неразрешимой задачей. Например, невозможно написать анализатор, по любому исходному коду определяющий, является ли анализируемая программа имплементацией алгоритма, вычисляющего, скажем, возведение в квадрат целого числа.
Таким образом, функциональность статических анализаторов имеет непреодолимые ограничения. Статический анализатор никогда не сможет во всех случаях определить такие вещи, как, например, возникновение «null pointer exception» в языках, допускающих значение null, или во всех случаях определить возникновение «attribute not found» в языках с динамической типизацией. Всё, что может самый совершенный статический анализатор — это выделять частные случаи, число которых среди всех возможных проблем с вашим исходным кодом является, без преувеличения, каплей в море.
Статический анализ — это не поиск багов
Из вышесказанного следует вывод: статический анализ — это не средство уменьшения количества дефектов в программе. Рискну утверждать: будучи впервые применён к вашему проекту, он найдёт в коде «занятные» места, но, скорее всего, не найдёт никаких дефектов, влияющих на качество работы вашей программы.
Примеры дефектов, автоматически найденных анализаторами, впечатляют, но не следует забывать, что эти примеры найдены при помощи сканирования большого набора больших кодовых баз. По такому же принципу взломщики, имеющие возможность перебрать несколько простых паролей на большом количестве аккаунтов, в конце концов находят те аккаунты, на которых стоит простой пароль.
Значит ли это, что статический анализ не надо применять? Конечно, нет! И ровно по той же причине, по которой стоит проверять каждый новый пароль на попадание в стоп-лист «простых» паролей.
Статический анализ — это больше, чем поиск багов
На самом деле, практически решаемые анализом задачи гораздо шире. Ведь в целом статический анализ — это любая проверка исходников, осуществляемая до их запуска. Вот некоторые вещи, которые можно делать:
- Проверка стиля кодирования в широком смысле этого слова. Сюда входит как проверка форматирования, так и поиск использования пустых/лишних скобок, установка, пороговых значений на метрики вроде количества строк / цикломатической сложности метода и т. д. — всего, что потенциально затрудняет читаемость и поддерживаемость кода. В Java таким инструментом является Checkstyle, в Python — flake8. Программы такого класса обычно называются «линтеры».
- Анализу может подвергаться не только исполняемый код. Файлы ресурсов, такие как JSON, YAML, XML, .properties могут (и должны!) быть автоматически проверяемы на валидность. Ведь лучше узнать о том, что из-за каких-нибудь непарных кавычек нарушена структура JSON на раннем этапе автоматической проверки Pull Request, чем при исполнении тестов или в Run time? Соответствующие инструменты имеются: например, YAMLlint, JSONLint.
- Компиляция (или парсинг для динамических языков программирования) — это тоже вид статического анализа. Как правило, компиляторы способны выдавать предупреждения, сигнализирующие о проблемах с качеством исходного кода, и их не следует игнорировать.
- Иногда компиляция — это не только компиляция исполняемого кода. Например, если у вас документация в формате AsciiDoctor, то в момент превращения её в HTML/PDF обработчик AsciiDoctor (Maven plugin) может выдавать предупреждения, например, о нарушенных внутренних ссылках. И это — весомый повод не принять Pull Request с изменениями документации.
- Проверка правописания — тоже вид статического анализа. Утилита aspell способна проверять правописание не только в документации, но и в исходных кодах программ (комментариях и литералах) на разных языках программирования, включая C/C++, Java и Python. Ошибка правописания в пользовательском интерфейсе или документации — это тоже дефект!
- Конфигурационные тесты (о том, что это такое — см. этот и этот доклады), хотя и выполняются в среде выполнения модульных тестов типа pytest, на самом деле также являются разновидностью статического анализа, т. к. не выполняют исходные коды в процессе своего выполнения.
Как видим, поиск багов в этом списке занимает наименее важную роль, а всё остальное доступно путём использования бесплатных open source инструментов.
Какие из этих типов статического анализа следует применять в вашем проекте? Конечно, все, чем больше — тем лучше! Главное, внедрить это правильно, о чём и пойдёт речь дальше.
Конвейер поставки как многоступенчатый фильтр и статический анализ как его первый каскад
Классической метафорой непрерывной интеграции является трубопровод (pipeline), по которому протекают изменения — от изменения исходного кода до поставки в production. Стандартная последовательность этапов этого конвейера выглядит так:
- статический анализ
- компиляция
- модульные тесты
- интеграционные тесты
- UI тесты
- ручная проверка
Изменения, забракованные на N-ом этапе конвейера, не передаются на этап N+1.
Почему именно так, а не иначе? В той части конвейера, которая касается тестирования, тестировщики узнают широко известную пирамиду тестирования.
Тестовая пирамида. Источник: статья Мартина Фаулера.
В нижней части этой пирамиды расположены тесты, которые легче писать, которые быстрее выполняются и не имеют тенденции к ложному срабатыванию. Потому их должно быть больше, они должны покрывать больше кода и выполняться первыми. В верхней части пирамиды всё обстоит наоборот, поэтому количество интеграционных и UI тестов должно быть уменьшено до необходимого минимума. Человек в этой цепочке — самый дорогой, медленный и ненадёжный ресурс, поэтому он находится в самом конце и выполняет работу только в том случае, если предыдущие этапы не обнаружили никаких дефектов. Однако по тем же самым принципам строится конвейер и в частях, не связанных непосредственно с тестированием!
Я бы хотел предложить аналогию в виде многокаскадной системы фильтрации воды. На вход подаётся грязная вода (изменения с дефектами), на выходе мы должны получить чистую воду, все нежелательные загрязнения в которой отсеяны.
Многоступенчатый фильтр. Источник: Wikimedia Commons
Как известно, очищающие фильтры проектируются так, что каждый следующий каскад может отсеивать всё более мелкую фракцию загрязнений. При этом каскады более грубой очистки имеют бОльшую пропускную способность и меньшую стоимость. В нашей аналогии это означает, что входные quality gates имеют бОльшее быстродействие, требуют меньше усилий для запуска и сами по себе более неприхотливы в работе — и именно в такой последовательности они и выстроены. Роль статического анализа, который, как мы теперь понимаем, способен отсеять лишь самые грубые дефекты — это роль решётки-«грязевика» в самом начале каскада фильтров.
Статический анализ сам по себе не улучшает качество конечного продукта, как «грязевик» не делает воду питьевой. И тем не менее, в общей связке с другими элементами конвейера его важность очевидна. Хотя в многокаскадном фильтре выходные каскады потенциально способны уловить всё то же, что и входные — ясно, к каким последствиям приведёт попытка обойтись одними лишь каскадами тонкой очистки, без входных каскадов.
Цель «грязевика» — разгрузить последующие каскады от улавливания совсем уж грубых дефектов. Например, как минимум, человек, производящий code review, не должен отвлекаться на неправильно отформатированный код и нарушение установленных норм кодирования (вроде лишних скобок или слишком глубоко вложенных ветвлений). Баги вроде NPE должны улавливаться модульными тестами, но если ещё до теста анализатор нам указывает на то, что баг должен неминуемо произойти — это значительно ускорит его исправление.
Полагаю, теперь ясно, почему статический анализ не улучшает качество продукта, если применяется эпизодически, и должен применяться постоянно для отсеивания изменений с грубыми дефектами. Вопрос, улучшит ли применение статического анализатора качество вашего продукта, примерно эквивалентен вопросу «улучшатся ли питьевые качества воды, взятой из грязного водоёма, если её пропустить через дуршлаг?»
Внедрение в legacy-проект
Важный практический вопрос: как внедрить статический анализ в процесс непрерывной интеграции в качестве «quality gate»? В случае с автоматическими тестами всё очевидно: есть набор тестов, падение любого из них — достаточное основание считать, что сборка не прошла quality gate. Попытка таким же образом установить gate по результатам статического анализа проваливается: на legacy-коде предупреждений анализа слишком много, полностью игнорировать их не хочется, но и останавливать поставку продукта только потому, что в нём есть предупреждения анализатора, невозможно.
Будучи применён впервые, на любом проекте анализатор выдаёт огромное количество предупреждений, подавляющее большинство которых не имеют отношения к правильному функционированию продукта. Исправлять сразу все эти замечания невозможно, а многие — и не нужно. В конце концов, мы же знаем, что наш продукт в целом работает, и до внедрения статического анализа!
В итоге, многие ограничиваются эпизодическим использованием статического анализа, либо используют его лишь в режиме информирования, когда при сборке просто выдаётся отчёт анализатора. Это эквивалентно отсутствию всякого анализа, потому что если у нас уже имеется множество предупреждений, то возникновение ещё одного (сколь угодно серьёзного) при изменении кода остаётся незамеченным.
Известны следующие способы введения quality gates:
- Установка лимита общего количества предупреждений или количества предупреждений, делённого на количество строк кода. Работает это плохо, т. к. такой gate свободно пропускает изменения с новыми дефектами, пока их лимит не превышен.
- Фиксация, в определённый момент, всех старых предупреждений в коде как игнорируемых, и отказ в сборке при возникновении новых предупреждений. Такую функциональность предоставляет PVS-studio и некоторые онлайн-ресурсы, например, Codacy. Мне не довелось работать в PVS-studio, что касается моего опыта с Codacy, то основная их проблема заключается в том, что определение что есть «старая», а что «новая» ошибка — довольно сложный и не всегда правильно работающий алгоритм, особенно если файлы сильно изменяются или переименовываются. На моей памяти Codacy мог пропускать в пулл-реквесте новые предупреждения, и в то же время не пропускать pull request из-за предупреждений, не относящихся к изменениям в коде данного PR.
- На мой взгляд, наиболее эффективным решением является описанный в книге Continuous Delivery «метод храповика» («ratcheting»). Основная идея заключается в том, что свойством каждого релиза является количество предупреждений статического анализа, и допускаются лишь такие изменения, которые общее количество предупреждений не увеличивают.
Храповик
Работает это таким образом:
- На первоначальном этапе реализуется запись в метаданных о релизе количества предупреждений в коде, найденных анализаторами. Таким образом, при сборке основной ветки в ваш менеджер репозиториев записывается не просто «релиз 7.0.2», но «релиз 7.0.2, содержащий 100500 Checkstyle-предупреждений». Если вы используете продвинутый менеджер репозиториев (такой как Artifactory), сохранить такие метаданные о вашем релизе легко.
- Теперь каждый pull request при сборке сравнивает количество получающихся предупреждений с тем, какое количество имеется в текущем релизе. Если PR приводит к увеличению этого числа, то код не проходит quality gate по статическому анализу. Если количество предупреждений уменьшается или не изменяется — то проходит.
- При следующем релизе пересчитанное количество предупреждений будет вновь записано в метаданные релиза.
Так понемногу, но неуклонно (как при работе храповика), число предупреждений будет стремиться к нулю. Конечно, систему можно обмануть, внеся новое предупреждение, но исправив чужое. Это нормально, т. к. на длинной дистанции даёт результат: предупреждения исправляются, как правило, не по одиночке, а сразу группой определённого типа, и все легко-устраняемые предупреждения довольно быстро оказываются устранены.
На этом графике показано общее количество Checkstyle-предупреждений за полгода работы такого «храповика» на одном из наших OpenSource проектов. Количество предупреждений уменьшилось на порядок, причём произошло это естественным образом, параллельно с разработкой продукта!
Я применяю модифицированную версию этого метода, отдельно подсчитывая предупреждения в разбивке по модулям проекта и инструментам анализа, формируемый при этом YAML-файл с метаданными о сборке выглядит примерно следующим образом:
celesta-sql:
checkstyle: 434
spotbugs: 45
celesta-core:
checkstyle: 206
spotbugs: 13
celesta-maven-plugin:
checkstyle: 19
spotbugs: 0
celesta-unit:
checkstyle: 0
spotbugs: 0
В любой продвинутой CI-системе «храповик» можно реализовать для любых инструментов статического анализа, не полагаясь на плагины и сторонние инструменты. Каждый из анализаторов выдаёт свой отчёт в простом текстовом или XML формате, легко поддающемся анализу. Остаётся прописать только необходимую логику в CI-скрипте. Подсмотреть, как это реализовано в наших open source проектах на базе Jenkins и Artifactory, можно здесь или здесь. Оба примера зависят от библиотеки ratchetlib: метод
countWarnings() обычным образом подсчитывает xml-тэги в файлах, формируемых Checkstyle и Spotbugs, а compareWarningMaps() реализует тот самый храповик, выбрасывая ошибку в случае, когда количество предупреждений в какой-либо из категорий повышается.Интересный вариант реализации «храповика» возможен для анализа правописания комментариев, текстовых литералов и документации с помощью aspell. Как известно, при проверке правописания не все неизвестные стандартному словарю слова являются неправильными, они могут быть добавлены в пользовательский словарь. Если сделать пользовательский словарь частью исходного кода проекта, то quality gate по правописанию может быть сформулирован таким образом: выполнение aspell со стандартным и пользовательским словарём не должно находить никаких ошибок правописания.
О важности фиксации версии анализатора
В заключение нужно отметить следующее: каким бы образом вы бы ни внедряли анализ в ваш конвейер поставки, версия анализатора должна быть фиксирована. Если допустить самопроизвольное обновление анализатора, то при сборке очередного pull request могут «всплыть» новые дефекты, которые не связаны с изменением кода, а связаны с тем, что новый анализатор просто способен находить больше дефектов — и это поломает вам процесс приёмки pull request-ов. Апгрейд анализатора должен быть осознанным действием. Впрочем, жёсткая фиксация версии каждой компоненты сборки — это в целом необходимое требование и тема для отдельного разговора.
Выводы
- Статический анализ не найдёт вам баги и не улучшит качество вашего продукта в результате однократного применения. Положительный эффект для качества даёт лишь его постоянное применение в процессе поставки.
- Поиск багов вообще не является главной задачей анализа, подавляющее большинство полезных функций доступно в opensource инструментах.
- Внедряйте quality gates по результатам статического анализа на самом первом этапе конвейера поставки, используя «храповик» для legacy-кода.
Ссылки
- Continuous Delivery
- А. Кудрявцев: Анализ программ: как понять, что ты хороший программист доклад о разных методах анализа кода (не только статическом!)
Комментарии (32)
EvgeniyRyzhkov
21.01.2019 11:38+2Иван, спасибо за статью и за то, что помогаете нам делать нашу работу – популяризовать технологию статического анализа кода.
Вы абсолютно правы, что статьи из блога PVS-Studio действуя на неокрепшие умы могут привести именно к таким выводам, что «проверю разок, исправлю ошибки и будет хорошо».
Это моя личная боль, которую я не знаю как победить уже несколько лет. Дело в том, что статьи про проверку проектов:
- Вызывают вау-эффект у людей. Людям нравится читать как косячат разработчики в Google, Epic Games, Microsoft и других компаниях. Людям приятно думать, что не только они сами ошибаются, но и лидеры индустрии делают ошибки. Люди любят читать такие статьи.
- Можно писать на потоке, к тому же не сильно думая. Я не хочу оскорбить конечно же наших ребят, которые пишут эти статьи. Но придумывать каждый раз принципиально новую статью намного сложнее, чем написать статью о проверке проекта (десяток ошибок, пару шуток, разбавить картинками единорогов).
Вы написали очень хорошую статью. У меня тоже есть парочка статей на эту тему. И у других коллег. Более того, я езжу с докладом по компаниям с темой «Философия статического анализа кода», где рассказываю именно про процесс, а не конкретные найденные ошибки.
Но не возможно написать 10 статей про процесс. А нам для популяризации продукта надо писать регулярно и много.
Прокомментирую еще несколько моментов из статьи отдельным комментариев, чтобы удобнее дискуссию вести было.EvgeniyRyzhkov
21.01.2019 12:03Вот про «Философию статического анализа кода», о которой я рассказываю во время поездок по компаниям, в виде небольшой статьи.
IvanPonomarev Автор
21.01.2019 17:30Евгений, большое спасибо за содержательный отзыв на статью! Да, мою озабоченность насчёт «действия на неокрепшие умы» в моём посте Вы уловили совершенно правильно!!!
Причём обвинять-то здесь никого нельзя, т. к. авторы статей/докладов про анализаторы не имеют задачи делать статьи / доклады про анализ. Но после недавних постов Andrey2008 и lany я решил, что больше не могу молчать )))
EvgeniyRyzhkov
21.01.2019 11:55+2Иван, как и писал выше, прокомментирую три момента из статьи. Те, что не комментирую – я с ними согласен значит.
Стандартная последовательность этапов этого конвейера выглядит так
Я не согласен, что первый шаг – это статический анализ, а только второй компиляция. Я считаю, что проверка компиляции в среднем быстрее и логичнее, чем сразу гонять «более тяжелый» статический анализ. Можем подискутировать, если думаете иначе.
Мне не довелось работать в PVS-studio, что касается моего опыта с Codacy, то основная их проблема заключается в том, что определение что есть «старая», а что «новая» ошибка — довольно сложный и не всегда правильно работающий алгоритм, особенно если файлы сильно изменяются или переименовываются.
В PVS-Studio это сделано офигенно удобно. Это одна из киллер-фич продукта, про которую, к сожалению, трудно писать статьи, поэтому ее мало знают. Мы сохраняем в базу информацию о имеющихся ошибках. Причем не просто «имя файла и строка», но и дополнительную информацию (хеши трех строк – текущая, предыдущая, следующая), чтобы в случае сдвига фрагмента кода мы его все-равно могли найти. Таким образом при незначительных модификациях мы все-равно понимаем, что это старая ошибка. И не ругаемся на нее. И теперь кто-то может сказать: «Ага, а если код сильно изменился, то значит это не сработает, и вы на него ругаетесь как на новый?» Да. Ругаемся. Но это и есть новый код. Если код сильно изменился, то это новый код, а не старый.
Благодаря этой фиче мы лично участвовали во внедрении в проект с 10 млн строк C++ кода, который каждый день «трогает» куча программистов. Все без проблем прошло.
Так что мы рекомендуем использовать эту фичу в PVS-Studio всем, кто внедряет статический анализ в свой процесс.
Вариант с фиксацией количества срабатываний относительно релиза – мне он намного менее симпатичен…
каким бы образом вы бы ни внедряли анализ в ваш конвейер поставки, версия анализатора должна быть фиксирована.
Не могу согласится. Категорический противник этого подхода. Я рекомендую всем обновляться на анализатор в автоматическом режиме. Так как мы добавляем новые диагностики и улучшаем старые. Почему? Во-первых, вы получите срабатывания на новые реальные ошибки. Во-вторых, часть старых срабатываний возможно пропадет, если мы их побороли.
Не обновлять анализатор – это как не обновлять антивирусные базы («А вдруг начнут про вирусы писать»). Не будем правда здесь обсуждать целесообразность антивирусов в целом.
Если после обновления версии анализатора у вас много новых срабатываний – подавите их, как я писал выше через эту функцию. Но не обновлять версию… Как правило, такие клиенты (они конечно есть) не обновляют версию годами. Им все некогда. Они ПЛАТЯТ за продление лицензии, но не пользуются новыми версиями. Почему? Потому что когда-то они решили зафиксировать версию. Но продукт сегодня и продукт три года назад – это небо и земля. А так получается «куплю билет, но не поеду».
VolCh
21.01.2019 14:20Речь всё-таки об автоматизированной CI больше, а в ней считается хорошей практикой иметь воспроизводимость билдов: два билда запущенных на одних и тех же исходниках (обычно на одном и том же коммите в билде) должны давать один и тот же результат даже если год между билдами прошёл (это и на дев-машинах хорошо, но в автоматических шагах CI просто маст хэв)
IvanPonomarev Автор
21.01.2019 17:59Да, совершенно верно, проблема здесь в воспроизводимости билдов.
EvgeniyRyzhkov, вот Вам реальный пример из опыта. Посмотрите вот этот коммит. Откуда он взялся? Мы поставили линтеры в TravisCI-скрипт. Они там работали как quality gates. Но внезапно, когда вышла новая версия Ansible-lint, которая стала находить больше ворнингов, у народа стали фейлится сборки пуллреквестов из-за ворнингов в коде, который они не меняли!!! В итоге был поломан процесс и срочные пуллреквесты стали мёржить без прохождения quality gates.
Никто не говорит, что не надо обновлять анализаторы. Конечно, надо! Как все другие компоненты сборки. Но это должен быть осознанный процесс, отражённый в исходном коде. И всякий раз действия будут по обстоятельствам (исправляем ли мы при этом вновь найденные предупреждения или просто ресетим «храповик»)lany
21.01.2019 18:37Как насчёт чтобы в условном build.gradle версия была указана конкретная, но при появлении новой версии робот на CI сам обновляет этот файл и коммитит обновлённый build-скрипт, возможно, разрешая на следующей сборке храповику прокрутиться назад? Тогда будет и обновление, и воспроизводимость.
IvanPonomarev Автор
21.01.2019 20:06Идея норм! По сути, это будет автоматизация ручной операции обновления версии анализатора, разрешать храповику прокручиваться назад при такой операции допустимо. Автоматизации нет предела )
IvanPonomarev Автор
21.01.2019 20:09Только если это серьёзно автоматизировать, надо учитывать вероятность того, что новая версия анализатора может оказаться неработоспособной, и откатывать изменение, если что-то пошло не так (иначе опять поломаем все билды). Но в целом да, наверное такое возможно.
EvgeniyRyzhkov
21.01.2019 22:40+1Когда меня спрашивают: «А есть ли в PVS-Studio возможность проверять каждый коммит?», то я отвечаю, что есть. И тут же добавляю: «Только ради бога не фейлите билд, если PVS-Studio что-то найдет!» Потому что иначе рано или поздно PVS-Studio станут воспринимать как мешающую штуку. И бывают ситуации, когда НАДО закоммитить быстро, а не сражаться с инструментами, не пропускающими коммит.
Мое мнение — фейлить билд в этом случае очень плохо. Хорошо рассылать сообщения почтой автором проблемного кода.IvanPonomarev Автор
22.01.2019 12:24Мое мнение — не бывает такого, что «надо быстро закоммитить». Бывает такое, что нет отлаженного процесса.
При хорошем процессе скорость возникает сама собой и не за счёт того, что мы взламываем процесс/кволити гейты, когда надо «по-быстрому».
Это не противоречит тому, что мы можем не фейлить билд на некоторых классах находок статического анализа. Это просто значит, что гейт настроен так, что определённые типы находок игнорируются, а для других находок у нас — Zero Tolerance.
Мой любимый commitstrip на тему «по-быстрому»: twitter.com/commitstrip/status/932673606840127489
IvanPonomarev Автор
21.01.2019 17:521. Тут Вы правы. Готов согласиться с компилятором/парсером вначале, и даже пожалуй это стоит поменять в статье! Например, пресловутый spotbugs по-другому и не может, т. к. анализирует байт-код. Есть экзотические случаи, например, в пайплайне для Ansible playbooks статический анализ лучше ставить перед парсингом, т. к. там он легковеснее. Но это именно что экзотика )
2. Вариант с фиксацией количества срабатываний относительно релиза – мне он намного менее симпатичен… — ну да, он менее симпатичен, менее техничен, зато очень практичен :-) Главное — это общий метод, руководствуясь которым я, имея любую кодовую базу и любой анализатор (не обязательно ваш), используяпалки и желудиgroovy и shell скрипты на CI, могу эффективно внедрить статанализ — где угодно, в сколь угодно страшном проекте. Кстати, вот сейчас мы подсчитываем ворнинги в разбивке по модулям и тулам, но если разбить ещё более гранулированно (по файлам), то это будет ещё ближе к методу сравнения старых/новых. Но у нас прижилось так, и я как-то полюбил этот ratcheting за то, что он стимулирует разрабов следить за общим количеством ворнингов и сбивать это количество потихоньку. Если бы был метод старых/новых, стимулировало ли бы это разрабов следить за кривой количества ворнингов? — может быть, да, может, нет.
По п. 3 ответили ниже, сейчас продолжу ответ в той ветке.EvgeniyRyzhkov
21.01.2019 22:42Я категорический противник подхода использовать старую версию анализатора. Что делать, если пользователь нашел баг в этой версии? Он отпишет его разработчику инструмента, и разработчик инструмента его даже поправит. Но в новой версии. Никто не будет поддерживать старую версию специально для некоторых клиентов. Если это не контракты на миллионы долларов.
IvanPonomarev Автор
22.01.2019 12:33Евгений, так речь же не про то совсем. Никто не говорит, то их надо старыми держать. Речь идёт фиксации версий зависимостей компонент сборки для их контролируемого обновления — это общая дисциплина, она касается всего вообще, и библиотек, и инструментов.
EvgeniyRyzhkov
22.01.2019 12:51Я понимаю «как надо теоретически». Но я вижу среди клиентов только два варианта. Или сидим на новой, или сидимо на старой. Такого что «у нас дисциплина и мы отсаем от текущей версии на два релиза» ПОЧТИ не бывает. Мне сейчас не важно сказать плохо это или хорошо. Я просто говорю о том, что вижу.
IvanPonomarev Автор
22.01.2019 13:03Я понял. Ещё ж всё сильно зависит о того, какие у ваших клиентов инструменты / процессы, и как их используют. Я, например, ничего не знаю про то, как бывает в мире C++)
Kobalt_x
21.01.2019 21:58В PVS-Studio это сделано офигенно удобно. Это одна из киллер-фич продукта, про которую, к сожалению, трудно писать статьи, поэтому ее мало знают. Мы сохраняем в базу информацию о имеющихся ошибках. Причем не просто «имя файла и строка», но и дополнительную информацию (хеши трех строк – текущая, предыдущая, следующая), чтобы в случае сдвига фрагмента кода мы его все-равно могли найти.
Да сделали вы замечательно, не спорю, да и в целом продукт замечательный,
Вот только, имхо эта фича меня немного раздражает, а именно, если при рефакторинге файл/кусок кода мигрирует из проекта в проект, то после регулярного анализа получаем по +1/+2 бестолковых комитта вида "update PVS-studio suppression db"(чтобы анализ на сервере пошел нормально). Потому что он не только находит но ещё и обновляет чтобы быстрее анализировалось, понятно что это для скорости. Но, если бы была настроечка включающая одну базу на sln/CmakeLists то было бы имхо попроще.
Paull
22.01.2019 10:07Потому что он не только находит но ещё и обновляет чтобы быстрее анализировалось, понятно что это для скорости. Но, если бы была настроечка включающая одну базу на sln/CmakeLists то было бы имхо попроще.
У нас можно добавлять suppress файл как на уровень sln (в случае Visual Studio проектов), так и при использовании прямой CMake интеграции (с нашим CMake модулем) Suppress файлы, однако, работают на уровне исходных файлов, поэтому если кусок кода переедет из одного файла в другой, то diff всё равно вылезет.
Также, если sln генерится CMake'ом, то добавлять в него suppress может быть неудобно — в таком случае можно в каждый ваш проект добавить общий suppress файл (т.е. все проекты будут ссылаться на один файл), а чтобы не модифицировать каждый проект по-отдельности, suppress файлы можно добавлять в проекты через общие MSBuild props'ы.
lany
23.01.2019 09:55> Я не согласен, что первый шаг – это статический анализ, а только второй компиляция. Я считаю, что проверка компиляции в среднем быстрее и логичнее, чем сразу гонять «более тяжелый» статический анализ.
В моей практике эти два шага идут одновременно и неразрывно с редактированием кода. После каждой правки кода автоматически в фоне IDE находит и ошибки компиляции (не компилируя на самом деле, но это и не важно) и предупреждения статического анализа. Там есть расстановка приоритетов, хайлайтинг ошибок обычно вперёд успевает завершиться, поэтому формально «сперва компиляция». Но обычно разница во времени меньше секунды, для человека это непринципиально.EvgeniyRyzhkov
23.01.2019 10:15Я только за, чтобы разработчики использовали статический анализ во время написания кода. И конечно же в PVS-Studio есть такая фича.
Но по моему опыту часть людей пока не готова к этому. Они хотят иметь только лишь билд-сервер, который рассылает результаты анализа.
Поэтому я и написал, что сначала компиляция.
А так — да, наибольшая эффективность от статического анализа как процесса достигается именно при использовании в IDE. Когда совсем нет траты времени человека на переключение контекста.IvanPonomarev Автор
23.01.2019 12:20Согласен! И хорошо, что у нас, джавистов, есть горячо любимая IDEA ))
IvanPonomarev Автор
23.01.2019 12:19+1Тут речь идёт скорее о том, что происходит в Jenkins/Travis в момент, когда открывается Pull Request. В нашей практике мы не можем навязывать всем разработчикам одну IDE: в конце концов, PR можно сделать, хоть непосредственно отредактировав исходник через веб-интерфейс GitHub.
При этом quality gates не должны пропустить лажу вне зависимости от того, из какого текстового редактора она пришла. И вот при коммите запускается этот скрипт, который как можно быстрее должен сработать, пока разработчик «держит фокус». В моей практике, тесты на PR могут идти до получаса (больше — нет смысла), но фейлиться (если есть, за что зафейлить) оно должно чем раньше, тем лучше…
amarao
21.01.2019 13:13+1К вопросу об опечатках. У меня есть проект, в котором для нужд тестирования бэкапов, инсталляция поднимается в LXC-контейнерах (на jenkins). Там есть скрипт
container-bootsrap.yaml. Да, я знаю, что в названии опечатка. Нет, я её не исправляю, потому что тогда придётся менять кучу скриптов (в т.ч. на машинах у разработчиков) и проще жить с "bootsrap'ом", чем с исправлением опечатки.
Так что, любите aspell. Но как его прикрутить к языку программирования, чтобы он не жаловался на тривиальное (
def alloc_data— это не две опечатки, а нормальное в программировании)?IvanPonomarev Автор
21.01.2019 18:07А я не проверяю aspell-ом названия файлов и методов. При проверке кода, Aspell выделяет и проверяет содержимое комментариев и строковых литералов. Основное применение Aspell — это спеллчекинг документации.
j8kin
21.01.2019 18:12+1Когда мы внедрили SonarQube в легаси проект, мы не использовали его результаты для прекращения или продолжнения сборки при CI, НО! Все найденные ошибки просматривались в формате: Zero Tolerance к ошибкам типа Bug, Critical и Major, были заведены задачи для фикса подобных ошибок в течении двух спринтов, однако стоит отметить, что на 30 тысяч строк кода их было в районе 50 (багов было вообще штучно 2 или 3 не помню точно). В течении 2 спринтов мы избавились от этих трех и далее уже взялись за Minor, количество которых держали в пределах 100.
И кстати мы в итоге включили блокировку merge если наличествовали ошибки первых 3 типов и более 100 minor. Именно процесс CI был в итоге оставлен прежним.
Считаю такой подход тоже хорош, так как позволяет обойтись без дополнительных тикетов по результатам CI. Проблемы фиксились еще разработчиком до коммита в основную ветку.IvanPonomarev Автор
21.01.2019 18:41Т. е. вы 1) взяли подмножество предупреждений, к которым решили применить Zero Tolerance 2) пофиксили их все и установили этот самый Zero Tolerance 3) остальные предупреждения ограничили простым гейтом по общему количеству. Да, вариант! Но не очень понимаю правда что такое «обойтись без дополнительных тикетов по результатам CI — проблемы фиксились еще разработчиком до коммита в основную ветку». Так ведь при любом описанном способе внедрения анализа дело даже даже не дойдёт до code review, пока quality gates не пропустят, не то что до merge в основную ветку.
j8kin
22.01.2019 13:09На сколько я понял статью, Code Analysis внедрялся как один из степов в пайплайне, т.е. выполнялся на бранче уже после выкладывания. Мы же использовали связку TFS+SonarQube, для того, чтобы делать Code Analysis во время вливания, т.е. изменения не вливались если изменения не надлежащего качества по мнению Sonar. А дальше выкатывание происходило отдельным процессом.
Возможно я не так понял процесс описанный в статье.IvanPonomarev Автор
22.01.2019 13:17Не совсем правильно поняли. Мы работаем по Github flow, и используем гейты при анализе Pull Request-ов, т. е. до их вливания в мастер. Т. е. кто-то делает PR, CI-сервер внутри себя выполняет «тестовое слияние» (без пуша в мастер) и прогоняет стат анализ и тесты. Если что-то из этого падает — PR «красный» и не может быть замёржен. Необходимое условие для вливания — «зелёный PR» + одобрение от ревьюеров.
Уже после вливания CI работает ещё раз, публикует сборку где надо и обновляет её метаданные.
411
21.01.2019 20:56На вход подаётся грязная вода
жестоко вы о чужом коде. Хотя эта фраза классно описывает ощущение от прочтения многих статей(не на хабре).
В целом статья хорошая, но зачастую стоит вопрос доступности таких решений для небольших проектов и стартапов, а также распространённости опыта интеграции данных решений среди разработчиков ввиду не самой большой доступности.IvanPonomarev Автор
22.01.2019 12:39Не понимаю, что значит «вопрос доступности».
Если речь идёт о стоимости программных компонент, то Jenkins бесплатен, Checktyle / Spotbugs / Aspell и куча других инструментов бесплатны, Artifactory Pro платен, но даже для небольших проектов это подъёмная сумма, GitHub/GitLab можно использовать бесплатно или за небольшие деньги для маленьких проектов.
Если Вы имеете в виду доступность знаний… ну достаточно просто зайти и посмотреть как реализован пайплайн в хороших опен-сорс проектах. Попробуйте туда покоммитить или хотя бы поизучайте их CI пайплайны.
SvyatoslavMC
Хорошая статья. Прокомментирую только несколько несколько мест исходя из реального опыта.
Обычно первый запуск даёт как раз самые сногсшибательные результаты на проекте, где ранее ничего не применялось.Думаю, совсем мимо. Примеры дефектов заранее известны, часто специально разрабатываются. Иногда собственный ляп в коде может стать примером для хорошей диагностики. Никакого абстрактного сканирования и переборов нет.
IvanPonomarev Автор
Спасибо за отзыв!
1. Ну это лотерея. Из моего опыта — где-то найдёт, где-то не найдёт. Я пытался донести в статье, что не стоит надеяться на одноразовое применение.
2. Я имел в виду примеры «реально найденных багов» из статей про анализаторы. Они конечно реально найденные, но искать их именно в том проекте никто не просил.