Skip to content

Conversation

@nixel2007
Copy link
Member

@nixel2007 nixel2007 force-pushed the feature/build-scripts branch from fe0f96d to 206049f Compare August 2, 2017 12:58
Copy link
Member

@artbear artbear left a comment

Choose a reason for hiding this comment

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

Хорошая работа.
Предлагаю сделать небольшие исправления.

Copy link
Member

Choose a reason for hiding this comment

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

На этапе после сборки также было бы удобно иметь доступ к АрхивПакета.
Предлагаю добавить.

Copy link
Member Author

Choose a reason for hiding this comment

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

готово

Copy link
Member

Choose a reason for hiding this comment

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

Предлагаю методы ВызватьСобытиеПриСборке и ВызватьСобытиеПослеСборки вставить сразу после метода ВызватьСобытиеПередСборкой, чтобы не разрывать текст и похожие блоки кода.

Copy link
Member Author

Choose a reason for hiding this comment

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

перенес

@artbear artbear added this to the 0.10 milestone Aug 2, 2017
Copy link
Member

@artbear artbear left a comment

Choose a reason for hiding this comment

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

Теперь все устраивает

@artbear
Copy link
Member

artbear commented Aug 2, 2017

@pumbaEO @EvilBeaver Нас с Никитой все устраивает, код-ревью и проверки сделали.
Если у вас нет вопросов, мержим и выпускаем релиз opm?

@EvilBeaver
Copy link
Member

У меня есть замечание. В opm уже предусмотрен клиентский кастомизирущий скрипт. Данный PR делает второй "спецфайл", но для этапа сборки по образу и подобию первого.

Однако, для разработчика уже есть клиентский кастомизирующий скрипт. Это, внезапно, packagedef.

Я помню дискуссию о том "код" это или все-таки "конфиг". Когда я задумывал opm и воровал packagedef с Ruby, я своровал оттуда и идею кодоконфига. По задумке - "перехватчик сборки" это и есть packagedef.

В принципе, если есть конкретные аргументы, почему это должен быть отдельный файл, я буду рад их услышать и принять.

Как вариант, можно было бы добавить в файл КонстантыOpm.ИмяФайлаСкриптаУстановки еще одну процедуру ПриСборке, а не плодить еще один файл с "волшебным" именем. Хотя это и не бог весть, какая хорошая идея.

Copy link
Member

@EvilBeaver EvilBeaver left a comment

Choose a reason for hiding this comment

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

Есть некоторый идейный вопрос

@nixel2007
Copy link
Member Author

@EvilBeaver основная проблема использования packagedef - он зачитывается при различных операциях внутри opm - установки по зависимостям описания пакета, установки самого пакета, сборки и прочих.

Это приводит к тому, что для vrunner, например, шаг сборки обработок (который добавлялся в packageDef для команды build) начал выполняться и при обычном opm install.

Вероятно частично могло бы помочь пробрасывание текущего режима (install/build/etc...) в контекст описания пакета, но и усложнения того же packagedef это бы добавило.

Предлагаемая реализация, имхо, наиболее компромисная со строгим разделением логики и ответственности между файлами.

@EvilBeaver
Copy link
Member

При opm install он зачитывается для генерации xml которую съедает потом vsc?

@nixel2007
Copy link
Member Author

nixel2007 commented Aug 2, 2017

Нет, из него зачитываются зависимости пакета и вызывается рекурсивное их разрешение/установка.

@EvilBeaver
Copy link
Member

Ну окай, я так-то не против.

@nixel2007 nixel2007 merged commit 23d2d02 into develop Aug 2, 2017
@nixel2007 nixel2007 deleted the feature/build-scripts branch August 2, 2017 15:20
@artbear
Copy link
Member

artbear commented Aug 2, 2017

А мне нравится возвращение к идее кодоконфига.

Может быть, прямо в packadef и определять спец.методы обработчики событий, аналогично build.os и install.os ?

Это вроде бы несложно сотворить.

@artbear artbear removed this from the 1.0.0 milestone Jan 11, 2018
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.

4 participants