Opera
Компания
57,25
рейтинг
29 октября 2012 в 22:25

Разработка → Встречайте Critic: система инспектирования кода в Opera Software

Внутренняя система инспектирования исходного кода Critic, применяемая в Opera Software, вчера вечером была выложена на Github под лицензией Apache License 2.0.

Иногда системы инспектирования кода ругают за то, что они совершенно не приспособлены к процессу коммерческой разработки. Это не про Critic. Critic опробован в процессе коммерческой разработки софта в больших проектах большой компании, и отлично себя показал. Очень рекомендую попробовать этот замечательный инструмент и вам.

Скачать исходные коды Critic можно здесь: github.com/jensl/critic.

Critic был написан шведским разработчиком Opera по имени Jens Lindström. Его не устраивали существующие системы инспектирования кода, поэтому он решил написать свою. Кстати, Йенс знаменит не только этим. Те, кому интересно, могут поискать в инете его посты про JS-движок Оперы Carakan.

Critic писался под конкретные нужды и задачи Оперы. Автор этого инструмента был также и его пользователем. Он старался сделать инструмент практичным и полезным и для себя, и для остальных разработчиков. Как пользователь Critic, наблюдавший его взросление и обрастание полезными фичами, я думаю, Йенсу это удалось. Critic получился удобным и эффективным инструментом, экономящим время своих пользователей.

Critic представляет из себя Web-приложение и написан на Python. Critic тесно интегрирован с Git. Ваши коммиты добавляются в review, как только вы протолкнули их в Git репозиторий, за которым наблюдает Critic.

Цикл инспектирования происходит следующим образом:
  1. Автор review коммитит свой код в Git-репозиторий, за которым наблюдает Critic.
  2. В зависимости от способа коммита, review может быть создано автоматически при git push, или вручную через Web-интерфейс Critic.
  3. Как только review создано — инспектора и наблюдатели за тем кодом, который подвергся изменению, будут уведомлены об этом по e-mail.
  4. Инспектора и наблюдатели могут добавлять записи о проблемах и заметки к коду. Только инспектора могут помечать изменённый код, как проинспектированный. Проинспектированный код — не значит одобренный. Это значит, что инспектор проверил его и создал записи обо всех проблемах, которые нашёл.
  5. Автор review участвует в обсуждении проблем и заметок, а также проталкивает в review новые коммиты, которые исправляют найденные проблемы. Новые коммиты помечаются, как непроинспектированные. Пункты 4-5 могут повторяться много раз.
  6. Когда все коммиты проинспектированы и не осталось ни одной открытой записи о проблемах — review считается одобреным.
  7. Одобренное review может быть переоткрыто, также как, например, баг в BTS. Закрытые записи о проблемах также могут быть переоткрыты. И конечно, можно добавить новые проблемы к уже проинспектированному коду. Философия — как в BTS. Есть определённый workflow, и ему обычно следуют, но если нужно — можно вернуться и на предыдущую фазу этого workflow.


Некоторые замечания:
  1. Записи о проблемах могут относиться ко всему review, к коммиту или к определённым строкам кода. Эти строки кода могут и не быть частью коммита, т.е. могут быть частью «нетронутого» кода, например, когда коммит меняет поведение нетронутого кода. Если запись о проблеме относится к определённым строкам кода и эти строки исправлены в следующем коммите — проблема помечается как «адресованная» этим коммитом. Новый коммит, однако, помечается как непроинспектированный, так что всё review не может автоматически стать одобреным. Это одна из фич, которая экономит время при работе с системой.
  2. Critic поддерживает «переписывание истории». Звучит страшно, но бывает полезно, особенно когда «переписывание» происходит на параллельной Git ветке через git rebase -i. Например, если ваше review разрослось до 50 коммитов, и многие из этих коммитов — мелочи вроде мелких багфиксов, фиксов компиляции, переименований переменных, редактирований комментариев и прочие «подчистки» — это замусорит вашу историю в VCS. В таких случаях бывает полезно «прилепить» все эти промежуточные фиксы к тем коммитам, которые они исправляют, прежде чем отдавать свою ветку на интеграцию в основной код проекта. Таким образом, в основной ветке проекта всегда будет красивая история, а не бардак. Ну, или почти всегда. Итак, если вы провели такую операцию и превратили 50 «плохих» коммитов в 5 «хороших», с тем же содержанием — Critic не будет просить проинспектировать этот код по второму разу. Чтобы всё получилось, суммарный diff 50 «плохих» коммитов должен быть 100% идентичен суммарному diff-у 5 «хороших» коммитов. Переписанная история будет опубликована в review.
  3. Critic также поддерживает смену точки ответвления review. Предположим, вы ответвили ветку review от вершины основной ветки проекта. За время инспектирования основная ветка проекта ушла вперёд. И вашей ветке надо изменить код в соответствии с новыми реалиями основной ветки. Что же делать, создавать новое review и терять всю уже проделанную работу над этим review? Как бы не так. Critic поможет вам и здесь. Все коммиты можно скопировать на новую точку ответвления, с помощью того же git rebase, и указать Critic, куда именно вы перенесли точку ответвления. И он всё поймёт! И не станет просить проинспектировать тот же код второй раз. И не попросит проинспектировать тонны кода из основной ветки. Зато заметит, что вы разрешили merge conflicts, и попросит инспекторов проверить, правильно ли вы это сделали. Ну разве не молодец?
  4. Critic поддерживает расширения. «Это же сейчас модно.» Расширения могут, очевидно, расширять функциональность Critic и ещё больше экономить ваше время. Расширения могут реализовывать различную специфическую функциональность, нужную не всем пользователям. Поэтому каждый пользователь сам устанавливает себе те расширения, которые ему нужны. Предположим, вы работаете над проектом Х. Фиксите баги. В review изначально был 1 коммит для багфикса, но инспектора попались очень строгие, и пришлось коммитнуть ещё 5 фиксов к фиксу, чтобы их удовлетворить. Наконец-то review одобрено. И вам предстоит немного простой, но нудной формальной работы по коммиту фикса. Кто-то с вашего проекта решил облегчить жизнь своим товарищам по команде и разработал расширение для вашего проекта. Расширение добавляет в Web-интерфейс кнопку «Интегрировать багфикс в проект Х». Теперь вы можете нажать сию волшебную кнопку, и Critic сделает для вас много полезного:
    • Соберёт ваши 6 коммитов в 1.
    • Предложит к нему комментарий от первого коммита.
    • Предложит поредактировать комментарий.
    • Проверит, нет ли merge conflicts с ушедшей вперёд основной веткой проекта, или веткой для багфиксов.
    • Закоммитит фикс в ветку для багфиксов.
    • Закроет review в Critic.
    • Напишет что-нибудь хорошее в BTS.
    • Закроет баг в BTS.

  5. Critic не является оболочкой вокруг основного репозитория проекта, как, например, Gerrit. Поэтому Critic можно использовать как для pre-commit review, так и для post-commit review, как удобно вашей организации. Critic, однако, может подтягивать код из основного репозитория, или делить с ним один репозиторий. А также толкать код туда с помощью расширений, как показано в предыдущем пункте.

Надеюсь, Critic будет полезен и вам. Удачи в разработке!
Автор: @alexeikh
Opera
рейтинг 57,25
Компания прекратила активность на сайте

Комментарии (34)

  • 0
    Из систем контроля версий поддерживается только git?
    • +2
      Судя по коду и списку зависимостей, да. А хотелось бы такую же плюшку для hg.
      • +1
        Действительно, поддерживается только git. Потому что Critic был изначально написан как внутренняя система для использования в Опере и в инфраструктуре Оперы. Решение о выпуске Critic «наружу» и открытии кода было принято позже. Потому что система получилась действительно хорошей, а, так как Critic не конкурирует с основным бизнесом Оперы, почему бы не выпустить её и не сделать мир лучше?

        Планов по добавлению других VCS, насколько я знаю, нет. Опере и Йенсу это просто не нужно. Кому нужно — могут собраться вместе и, собственно, дописать поддержку Mercurial или чего другого, благо исходники есть и лицензия позволяет.
  • +1
    Куча всевозможных возможноcтей, но скажите как им пользоваться? Хотя бы пару примеров и их результатов. Я как рядовой пользователь хочу увидеть результат (скриншоты, лог CLI) прежде чем качать и запускать скрипты.
    • 0
      Сейчас скачаю, кину ссылку для тестирования, если быстро настрою… Самому интересно что за штука))
      • 0
        Мда… безопасностью ребята не заморачивалсь, поэтому доступ не дам, и удалил всё.
        Ничего там интересного, прикручиваться надо основательно к какой-нибудь web-морде git-а.
        Пока очень слабо, тяжело конфигурировать, хотя установка «в одну команду».
        Пара снимков
        image
        image

        Тыкание в интерфейс не снимает вопрос «Как с этим работать?!».
        Ждём обновлений и нормальных манов, желательно в виде видео/презентации последовательности действий.
        • +2
          Большое спасибо за скриншоты! А то я уж думал, как и скриншоты сделать, и NDA соблюсти, и персональную копию Critic не ставить.

          > безопасностью ребята не заморачивалсь

          Что имеется в виду?

          > Ждём обновлений и нормальных манов

          Документации и правда немного, но она есть. Видите на скриншотах линк «Tutorial» сверху? Там вкратце, да, к сожалению вкратце, описаны наиболее популярные use-case-ы, как то создание review, rebasing, etc. То же самое можно почитать здесь: github.com/jensl/critic/tree/master/tutorials. Также есть немного документации здесь: github.com/jensl/critic/tree/master/documentation.

          Возможно, наш Developer Relations Team напишет статью или несколько на dev.opera.com/, чтоб было попонятней.

          И я могу на вопросы поотвечать, правда я сам никогда Critic не ставил и не админил, только пользовался. Можно начать, если интересно. У вас получилось создать review?
          • +1
            Вопрос с безопасностью… Как ограничивать доступ к ФС? Как назначить пользователя без прав редактирования? Как нормально назначать пользователей?
            Я догадался документацию почитать)) Да, получилось с review. Правильно понимаю концепцию, что при обновлении репов создаётся новая ветка? Вот с ветвлениями ничего не пойму (что когда и как!?)…
            И что с работай с репами, отслеживаемыми другими системами (мой случай — это директория gitolite, к которой прикручен, допиленный под себя ,gitlist, репы администрируются через плагин redmine)?
            Понял, что интеграция с web-интерфейсами git есть, а что кроме просмотра там делать не ясно.
            • +3
              > Как ограничивать доступ к ФС?

              Как я понимаю, имеется в виду доступ к ФС репозитория, и доступ туда есть только через git. Ну, тогда так же, как и просто к любому git-репозиториию. И Critic тут вообще не при делах. У меня такое чувство, что я не понял вопроса.

              UPD. А, или речь про то, что встроенный в Critic repository viewer будет показывать весь код, до которого сможет дотянуться сам? Это да, будет.

              > Как назначить пользователя без прав редактирования?

              Без прав редактирования review, то есть добавления проблем и замечаний? Это да, никак.

              > Правильно понимаю концепцию, что при обновлении репов создаётся новая ветка?

              «При обновлении репов»? Я бы сказал, «при создании review». Да, при создании review создаётся новая ветка. Та самая, с префиксом «r/».

              > И что с работай с репами, отслеживаемыми другими системами (мой случай — это директория gitolite, к которой прикручен, допиленный под себя ,gitlist, репы администрируются через плагин redmine)?

              Critic интересуют только ветки с префиксом «r/». Поэтому я не вижу причин для конфликтов с другим софтом, как то gitolite, gitlist или redmine.

              Кроме repo viewer-a, который, как я сказал, пользователям Critic покажет всё, что видит сам Critic, что может быть нежелательно.

              > Понял, что интеграция с web-интерфейсами git есть, а что кроме просмотра там делать не ясно.

              В repo viewer-e — разве что review создать. А вот в созданном review можно выделить мышкой один или несколько коммитов, в них найти изменённые файлы, в них найти и выделить мышкой строки кода, и Critic предложит создать проблему (issue) или замечание (note). А также напротив имён файлов есть галочки, которые если проставить — просмотренный файл в данных коммитах будет помечен как проинспекированный. Но эта галочка доступна только инспекторам (reviewers). Чтобы стать инспектором, надо или добавить себя явно, там в review есть кнопка типа «Add reviewers», или заранее добавить фильтр «для каталогов (думайте: модулей) таких-то назначать меня инспектором». Обычно ответственный за модуль назначает себя его инспектором 1 раз, и после этого ему приходят уведомления обо всех review, содержащих изменения кода в этом модуле, а также он автоматически назначается инспектором своих модулей в каждом новом review. Т.е. фильтры срабатывают при создании review.
              • +1
                Огромное спасибо за развёрнутый ответ! Теперь понимаю как могу использовать систему. Через недельку попинаю, если будет успешный опыт, то напишу пост.
                Концепция складывается так: дублировать issuse в redmine, создать правила (зарезервировать ключевые слова) для изменения статусов задач по коммитам, вынести note и review в субдиректорию URI, встроив (frame-ом?) в redmine.
                Остальное по ситуации додумаю…
  • 0
    а чем Critic лучше того code review, что встроен в GitHub?
    • НЛО прилетело и опубликовало эту надпись здесь
      • 0
        • 0
          и мешок бабла за него
          • +1
            Тут вопрос не только в деньгах, но и в доверии своего кода и его обсуждений сторонней компании. Даже если вы доверяете, что Github ничего плохого не сделает, вдруг его взломают?

            Также, в случае собственного сервера у организации больше контроля, во всех смыслах. Например, есть полный доступ к базам данных. В случае миграции на другую code review system (или другую VCS), есть возможность написать преобразователь данных из формата одной системы в формат другой. Можно дописать интеграцию с другим софтом, использующимся в компании. Да много всего.

            • 0
              github enterprise насколько я помню, поставляется как полностью настроенный образ системы, не? Т.е. какую угодно интеграцию, кроме как через апи и не сделаешь. Зато есть бесплатный аналог gitlab, который умеет все тоже, местами, возможно, даже лучше. Ставится на свой сервер. Имхо, 5 штук только за то, чтобы код на 100% остался цел и был у вас — неоправданные расходы, только если вы не огромная корпорация.
              • 0
                Да, и правда, Github Enterprise ставится внутри организации. И да, согласен, дороговато. А для огромной корпорации оно и дороже будет, там цена растёт линейно от количества пользователей.
    • +1
      А что там есть из функционала? Я заметил только, что можно комментировать строки кода. И можно комментировать одну и ту же строку несколько раз, таким образом «ведя беседу».

      К примеру, в Critic есть:
      * Понятие «review» наподобие бага в BTS.
      * Workflow для review: сoздание, инспектирование, создание записей о проблемах и замечаниях, работа над проблемами и замечаниями, одобрение, закрытие.
      * Понятия собственно «проблемы» и «замечания».
      * Отдельные потоки обсуждения каждой проблемы, если например, для тех же строк создано несколько «проблем».
      * Возможность привязывания проблемы и замечания к диапазону строк, а не к одной строке.

      Т.е. даже в базовом функционале Critic отличается от «Github reviewer-a», примерно как BTS отличается от обсуждения бага в мэйллисте.

      Ну и плюс ещё те плюшки, которые собственно описаны в статье.
  • +2
    А как оно хоть выглядит то?
  • +1
    Бесит когда вроде не старые проекты, но требуют apache в зависимостях. Только поэтому не стал ставить по инструкции. Колупаюсь в скриптах установки и отключаю все операции с апачем связанные. Сам потом задеплою через nginx + uwsgi.
    Раз уж проект на питоне написан то лучшеб сделали простенький сервер на питоне по умолчанию, и по желанию уже nginx/cherokee/lighty/apache итд — кому что нравится.
    • 0
      Вы разобрались как через интерфейс новых юзеров добавлять? Что-то там всё совсем-совсем не очевидно, похоже на набор скриптов, отсылающих уведомления и делающих ветвления. Назначение web-интерфейса, кажется, сведено к добавлению репов и редактирования уведомлений.
      • +1
        В Critic нет никакого экрана логина для пользователя. По крайней мере, я никогда не видел. Когда я пользуюсь Critic, я вхожу на сервер, используя HTTP authentication. Не знаю, кто его предоставляет, сам Critic или Apache.

        > Назначение web-интерфейса, кажется, сведено к добавлению репов и редактирования уведомлений.

        БОльшая часть web-интерфейса предназначена для работы над review. Но review должно существовать, чтобы над ним можно было работать. Если открытые review существуют, и вы на них подписаны — они находятся в Dashboard. Чтобы подписаться на review определённых каталогов — надо создать фильтр с каталогом, внутри которого находятся интересующие файлы. Каталог может быть "/", тогда произойдёт подписка на все каталоги и файлы. Чтобы создать review, надо затолкнуть ветку в git репозиторий Critic-a с префиксом «r/». Это возможно через web-интерфейс, или просто через git push.
  • +1
    Полезли с коллегой исходники смотреть… Образцово-показательный говнокод. Нет, спасибо, забирайте обратно.
    • +4
      Вы должны понимать, кто этот код писал :)

      Йенс Линдстрем — кодер один на миллион, у него чрезвычайная работоспособность. Кроме Каракана (или Джаракана) он написал самый быстрый HTML/XML парсер в мире и еще много других компонентов Оперы. Код он пишет очень быстро, тот же Каракан был готов за полгода, причем 90% работы Йенс сделал в одиночку. Еще через полгода движок Оперы опережал V8 по бенчмаркам. Напомню, что V8 писала команда из 10 человек в течение 4х лет, и некоторые из них — ветераны VMостроения.

      Но чудес не бывает: код часто оказывается грязным и, возможно, неидиоматичным. Тем более, Critic написан на Питоне, а основным языком для Йенса все-таки является C++. Однако, при всех недостатках багов в коде Йенса очень мало, и те компоненты Оперы, которых он касался, считаются наиболее надежными.
      • +1
        > Кроме Каракана (или Джаракана)
        Официально — «Чаракан» :)

        > Но чудес не бывает: код часто оказывается грязным и, возможно, неидиоматичным.
        Свободная лицензия как раз и призвана выступать в качестве лекарства :)
        • +1
          > Свободная лицензия как раз и призвана выступать в качестве лекарства :)

          Поддерживаю, ход правильный!
  • +1
    Расскажу про свой опыт и задам пару вопросов. Надеюсь alexeikh мне поможет.

    Мы давно в компании ищем подходящий инструмент для code review (git). Опробовали много всего и gerrit и crew (http://crew-cr.org/) и ещё кучу каких-то поделок. Сейчас пока остановились на встроенном в gitLab (http://gitlabhq.com/), так как GitLab мы используем для всего остального связанного с git.

    В общем, попробовали сегодня развернуть critic.
    Мы для этого сразу выделили отдельную вирт машину в нашем «облаке». На неё взгромоздили ubuntu server 12.04.

    Авторизацию сделали так: установили critic с авторизацией «host» (т.е. на совести Apache), в Apache включили авторизацию через mod_authnz_external + pwauth + authnx_unixgroup.

    Всё это наличиствует в пакетах, настройка ограничивается небольшими модификациями вирт. хоста для Apache, который генерит установщик critic:
    /etc/apache/sites-enables/critic-main
    <VirtualHost *:80>
    ServerAdmin your-mail@example.com
    ServerName host.name.example.com

    #setup unix auth
    AddExternalAuth pwauth /usr/sbin/pwauth
    SetExternalAuthMethod pwauth pipe

    WSGIApplicationGroup %{GLOBAL}
    WSGIProcessGroup critic-main
    WSGIDaemonProcess critic-main processes=2 \
    threads=25 \
    home=/usr/share/critic \
    python-path=/etc/critic/main:/usr/share/critic \
    user=critic \
    group=critic

    WSGIImportScript /usr/share/critic/wsgistartup.py \
    process-group=critic-main \
    application-group=%{GLOBAL}

    WSGIScriptAlias / /usr/share/critic/wsgi.py

    WSGIPassAuthorization Off

    <Directory "/usr/share/critic">
    AuthType Basic
    AuthName critic
    AuthBasicProvider external
    AuthExternal pwauth
    AuthzUnixgroup on
    Require valid-user
    Require group critic
    <[слеш]Directory>

    # Possible values include: debug, info, notice, warn, error, crit,
    # alert, emerg.
    LogLevel warn

    ErrorLog /var/log/critic/main/error.log
    CustomLog /var/log/critic/main/access.log combined

    Alias /static-resource/ "/usr/share/critic/resources/"
    <Directory "/usr/share/critic/resources">
    Options Indexes MultiViews FollowSymLinks
    AllowOverride None
    Order allow,deny
    Allow from all
    ExpiresActive On
    ExpiresDefault A2592000
    <[слеш]Directory>
    <[слеш]VirtualHost>


    (если кто-то будет брать за основу не забудьте [слеш] заменить на символ, в исходном виде это ломало тег spoiler).

    В итоге в critic попадают тем, кому создали аккаунт на сервере и добавили в группу critic. Этих же прав будет достаточно для push'инга веток на code review.

    Судя по тому, что приложение wsgi'ное позже apache мы возможно заменим чем-то более подходящим.

    Дальше настроили репозитории. Для этого пользователю critic сделали deploy ключ к нужным нам репозиториям в GitLab (технически для управления git там gitolite).

    Репозитории успешно загрузили, отправили тестовый review. Попробовали пописать комментов и issues. Всё в общем и целом не плохо пошло. Возможно нам хотелось бы немного поправить мелочи в интерефейсе и логике. При удачном стечении обстоятельств стоит ожидать от нас Merge Request'ов на GitHub'e.

    Но тут и возникает вопрос: как не бились так и не поняли, как по результатам review поправить код и добавить к review новые комиты? Нужно создать новую ветку? Что-то запушить в /r/user/branch или как?

    Буду благодарен за помощь. Так как чтение всего встоенного Tuturial и FAQ не дали ответа.
    • +1
      Нужно запушить новые коммиты в r/user/branch. В ветку с «r/» префиксом, это важно. Я так понимаю, при этом должен выпониться git pre-receive hook github.com/jensl/critic/blob/master/hooks/pre-receive, который и даст знать Critic-у про новый коммит.

      Хук то установили? Вот здесь дока, если что: git-scm.com/book/en/Customizing-Git-Git-Hooks.
      • 0
        Да, так мы уже пробовали, но коммит отклоняется с ошибкой типа:

        error: src refspec r/testdev/super-fix does not match any.
        error: failed to push some refs to 'testdev@example.com:/var/git/sandbox.git'

        Хук на месте, его создал сам critic в своих локальных версиях репозиториев.
        • 0
          Отбой. Разобрались :)

          Ошибка была в том, что мы использовали короткий формат, находясь при этом не в ветке '/r/...', а в исходной, которая послужила основой для неё.

          Спасибо за помощь.

          Кому интересно, шаги должны быть такие:
          git checkout super-fix
          nano bug-code.py
          git add bug-code.py
          git commit
          git push critic super-fix:r/testdev/super-fix
          
  • 0
    .
  • 0
    Отличный нейминг, коллеги!

Только зарегистрированные пользователи могут оставлять комментарии. Войдите, пожалуйста.

Самое читаемое Разработка