Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Исправил ошибку, которая приводила к удалению установленного пакета при его неудачном обновлении #185

Closed
wants to merge 4 commits into from
Closed

[WIP] Исправил ошибку, которая приводила к удалению установленного пакета при его неудачном обновлении #185

wants to merge 4 commits into from

Conversation

ovcharenko-di
Copy link
Contributor

@ovcharenko-di ovcharenko-di commented Oct 6, 2020 •
edited by nixel2007
Loading

close #176

  • исправил ошибку с некорректным вызовом РаботаСПакетами.УстановитьПакетИзФайла (был пропущен аргумент)
  • добавил параметр, позволяющий при неудачном обновлении пакета не удалять в целевом каталоге исходный пакет
  • добавил тест, имитирующий установку пакета с последующим его неудачным обновлением из-за меньшей версией среды

@ovcharenko-di
#close 176
6f4ff0d 
исправил ошибку с некорректным вызовом РаботаСПакетами.УстановитьПакетИзФайла (был пропущен аргумент)

добавил параметр, позволяющий при неудачном обновлении пакета не удалять в целевом каталоге исходный пакет

добавил тест, имитирующий установку пакета с последующим его неудачным обновлением из-за меньшей версией среды
@nixel2007 nixel2007 requested a review from khorevaa October 6, 2020 19:09
EvilBeaver
EvilBeaver reviewed Oct 7, 2020
View reviewed changes
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.

Есть вопросы

features/step_definitions/fixtures/testpackage_v999/folder/src.dll
@@ -0,0 +1 @@
// заглушка
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
Contributor Author

Choose a reason for hiding this comment

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

я скопировал каталог с тестовым пакетом, это все оттуда. Но в новом каталоге в packagedef изменил версию среды на заведомо недостижимую.

Есть ли более простой способ воспроизвести ситуацию, когда пакет обновляется на меньшей версии среды, чем требует обновленный манифест?

Copy link
Member

Choose a reason for hiding this comment

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

Не понял вопроса. Зачем нужен файл src.dll и как он связан с "воспроизведением ситуации" и обновлением манифестов

features/step_definitions/fixtures/testpackage_v999/folder/src.os
@@ -0,0 +1 @@
// заглушка
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
Contributor Author

Choose a reason for hiding this comment

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

см. выше

features/step_definitions/fixtures/testpackage_v999/packagedef
// КаталогУстановкиПакета - строка. Путь в который пакетный менеджер устанавливает текущий пакет.
// ЧтениеZipФайла - ЧтениеZipФайла. Архив пакета.
//
Процедура ПередУстановкой(Знач КаталогУстановкиПакета, Знач ЧтениеZipФайла) Экспорт
Copy link
Member

Choose a reason for hiding this comment

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

Требуется ли здесь полный packagedef со всеми процедурами?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

возможно, не требуется. попробую его упростить.

src/cmd/Классы/КомандаOpm_Update.os
@@ -49,11 +49,12 @@
НастройкаУстановки.УстанавливатьЗависимости = НеобходимУстановитьЗависимости;
НастройкаУстановки.СоздаватьФайлыЗапуска = СоздаватьФайлыЗапуска;
НастройкаУстановки.ИмяСервера = ИмяСервера;
НастройкаУстановки.УдалятьКаталогПриОшибкеУстановки = Ложь;
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
Contributor Author

Choose a reason for hiding this comment

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

Судя по истории, такое поведение закладывалось изначально. Не смогли установить - очистили каталог.
@artbear в свое время исправил баг, когда opm пытался очистить несуществующий каталог типа "" или Неопределено. При установке пакета такое поведение может и подходит, но вот при обновлении - нет.

Попутно обнаружил, что даже с учетом моего PR opm может отрабатывать некорректно, т.к. в процессе обновления часть файлов может успеть перезаписаться. По-моему, уж лучше случайно удалить библиотеку, чем "побить" ее. Займусь этим вопросом.

Попутно также обнаружил, что билды на CI уже давно не билдятся, исправлю сперва это.

Copy link
Member

Choose a reason for hiding this comment

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

по поводу ci - может заодно на GA переедем? пример работающих пайплайнов можно подсмотреть в entity, я ее недавно перевел.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Я за. Будет и win, и linux, и macOS сразу. Отдельным PR пульну.

src/core/Классы/МенеджерУстановкиПакетов.os
Процедура УстановитьПакетИзАрхива(Знач ФайлПакета, Знач ЭтоЗависимыйПакет = Ложь) Экспорт
Процедура УстановитьПакетИзАрхива(Знач ФайлПакета,
Знач ЭтоЗависимыйПакет = Ложь,
Знач УдалятьКаталогПриОшибкеУстановки = Истина) Экспорт
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
Contributor 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.

Я не понял, что написано выше (
Если мы удаляем каталог - надо удалять его всегда. Зачем нужен флаг, кто будет менять его значение?

nixel2007
nixel2007 reviewed Oct 7, 2020
View reviewed changes
src/core/Классы/МенеджерУстановкиПакетов.os
@@ -33,7 +33,8 @@

Если Не ВходящийКаталогУстановки = Неопределено Тогда
УстановитьЦелевойКаталог(ВходящийКаталогУстановки);
ИначеЕсли ТекущийРежимУстановкиПакетов = РежимУстановкиПакетов.Локально Тогда
ИначеЕсли ВходящийКаталогУстановки = Неопределено
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
Contributor Author

Choose a reason for hiding this comment

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

Вот поэтому и не надо в операторах ИначеЕсли одного уровня проверять совершенно разные условия. 😄

Сейчас видится, что правильнее вложить Если с режимом установки внутрь Если с входящим каталогом. Парой строчек ниже это фрагмента так и сделано. Исправлю.

@khorevaa
Copy link
Member

khorevaa commented Oct 9, 2020

@ovcharenko-di @nixel2007 @nixel2007 Коллеги - мне видится, что мы делаем заплатку.
На мой взгляд правильное поведение - это установка нового пакета в промежуточный каталог ( вместе с тем зависимостями, что добавляются или обновляются), а только потом уже перенос в рабочий "либ".

Хотя может я просто слишком замороченный :)

@ovcharenko-di
Copy link
Contributor Author

@ovcharenko-di @nixel2007 @nixel2007 Коллеги - мне видится, что мы делаем заплатку.
На мой взгляд правильное поведение - это установка нового пакета в промежуточный каталог ( вместе с тем зависимостями, что добавляются или обновляются), а только потом уже перенос в рабочий "либ".

Хотя может я просто слишком замороченный :)

Я именно так и планировал дорабатывать этот PR! Заодно посмотрю как в других менеджерах пакетов решается подобная проблема.

@EvilBeaver
Copy link
Member

Заодно посмотрю как в других менеджерах пакетов решается подобная проблема.

Так и решается. Кроме того, создается служебный .lock-файл, который в случае успех а- удаляется. Соответственно, если файл на месте - значит либо идет параллельный процесс установки, либо предыдущая упала и не является валидной.

@EvilBeaver
Copy link
Member

@ovcharenko-di мы же ждем доработки, да? Можешь в таком случае WIP поставить в заголовке?

@ovcharenko-di ovcharenko-di changed the title Исправил ошибку, которая приводила к удалению установленного пакета при его неудачном обновлении [WIP] Исправил ошибку, которая приводила к удалению установленного пакета при его неудачном обновлении Oct 14, 2020
@nixel2007 nixel2007 marked this pull request as draft October 15, 2020 11:17
@EvilBeaver
Copy link
Member

@ovcharenko-di подниму?

@ovcharenko-di
Copy link
Contributor Author

@EvilBeaver спасибо за пинг, я уже и забыл об этом PR!
Я постараюсь еще поработать над ним.

@artbear artbear deleted the branch oscript-library:develop April 6, 2023 12:34
@artbear artbear closed this Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixel2007 nixel2007 nixel2007 left review comments

@EvilBeaver EvilBeaver EvilBeaver left review comments

@khorevaa khorevaa Awaiting requested review from khorevaa

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ovcharenko-di @khorevaa @EvilBeaver @nixel2007 @artbear