При установке пакета через apt-get install также устанавливаются все его зависимости. Однако позже при удалении пакета все его зависимости остаются в системе, даже если в них больше нет необходимости. В debian-based системах реализованы команда apt-get autoremove и вспомогательная утилита apt-mark, вместе служащие для удаления пакетов, которые ранее были установлены автоматически для удовлетворения зависимостей пакетов, устанавливаемых пользователем, и которые при этом больше не требуются (т.е. все пакеты, зависящие от них, удалены или удаляются). Мануалы данных команд доступны по следующим URL: https://manpages.debian.org/jessie/apt/apt-get.8.en.html https://manpages.debian.org/jessie/apt/apt-mark.8.en.html
Сделанный мной вариант реализации доступен по следующему адресу: http://git.altlinux.org/people/darktemplar/packages/?p=apt.git;a=summary
К нему ещё нужен rpm: http://git.altlinux.org/people/darktemplar/packages/?p=rpm.git;a=summary
rpm исправлен, наверное можно и apt отправить.
смотреть изменения в apt. Решил попробовать записывать свои заметки в git notes. Рассматриваемые коммиты запушил к себе так: git push -u @ALT autoremove --follow-tags а сохранённые заметки (пока только одну) пришлось так: $ git push @ALT refs/notes/commits Counting objects: 6, done. Delta compression using up to 4 threads. Compressing objects: 100% (6/6), done. Writing objects: 100% (6/6), 805 bytes | 0 bytes/s, done. Total 6 (delta 1), reused 0 (delta 0) remote: gitery-update: Unrecognized ref name: refs/notes/commits remote: error: hook declined to update refs/notes/commits To git.alt:packages/apt.git ! [remote rejected] refs/notes/commits -> refs/notes/commits (hook declined) error: failed to push some refs to 'git.alt:packages/apt.git' $ git push @ALT refs/notes/commits:refs/heads/NOTES Counting objects: 6, done. Delta compression using up to 4 threads. Compressing objects: 100% (6/6), done. Writing objects: 100% (6/6), 805 bytes | 0 bytes/s, done. Total 6 (delta 1), reused 0 (delta 0) remote: gitery-sendmail: email notification about `refs/heads/NOTES' update sent. remote: Repacking repository... done remote: Updating committer date cache... done To git.alt:packages/apt.git * [new branch] refs/notes/commits -> NOTES $ Забрать к себе должно быть можно в обратную сторону: git fetch git://git.altlinux.org/people/imz/packages/apt.git refs/heads/NOTES:refs/notes/commits и посмотреть как-то так: git log gears/sisyphus..autoremove --reverse Тогда вы увидете первую заметку сначала. (При условии, что вы уже имеете ветку autoremove.) $ git log gears/sisyphus..autoremove --reverse | head -30 commit 5d970b58d5ec676ef543f678ab52d1921e3266b0 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Tue Oct 24 13:06:40 2017 +0300 Move some common functions out of apt-get.cc to library Replace legacy int types. Based on ideas and code from apt-1.4.8 from Debian. Notes: (TODO: review more thoroughly for new bugs.) Let's assume this is an equivalent rewrite of the code. 1. Perhaps it would be easier to review if non-similar changes were placed in separate commits (grouping the similar changes such as: "Replace legacy int types.") 2. Was it necessary to make the changes in spacing (which can be hidden with git diff -w)? If yes, it would be easier to review if they were in a separate commit. commit 48ff0be3b7ada042e0b1cd33a2998c223117f8fa Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Thu Oct 26 15:58:22 2017 +0300 Add skeleton of apt-mark Based on ideas and code from apt-1.4.8 from Debian. commit ca9af74c6d592a4d908b48d8a7bc413de0dd694b Когда сохранённых заметок будет больше или они будут интересными, пошлю их по почте или сюда тоже.
Мои заметки (см. "Notes" у кажого коммита); самые общие -- на последнем коммите. Там где-то написано, но ещё раз напишу, что трудно понимать, когда не сильно связанные вещи меняются в одном коммите. Ещё интересно: что так же, как в Debian? что исправлено/улучшено такого, что в Debian ещё не исправлено? $ git --no-pager log --reverse gears/sisyphus..autoremove commit 5d970b58d5ec676ef543f678ab52d1921e3266b0 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Tue Oct 24 13:06:40 2017 +0300 Move some common functions out of apt-get.cc to library Replace legacy int types. Based on ideas and code from apt-1.4.8 from Debian. Notes: (TODO: review more thoroughly for new bugs.) Let's assume this is an equivalent rewrite of the code. Effect on the old behavior: must be equivalent (not checked thoroughly). 1. Perhaps it would be easier to review if non-similar changes were placed in separate commits (grouping the similar changes such as: "Replace legacy int types.") 2. Was it necessary to make the changes in spacing (which can be hidden with git diff -w)? If yes, it would be easier to review if they were in a separate commit. commit 48ff0be3b7ada042e0b1cd33a2998c223117f8fa Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Thu Oct 26 15:58:22 2017 +0300 Add skeleton of apt-mark Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old behavior: This does not change the behavior of old commands, because the calls to read/writeStateFile() do nothing as of now. 1. I don't understand the comment and the following code yet: // In apt-get it's called 'Dir::State::apt_mark_storage', but format used here is incompatible, due to that name is different Cnf.CndSet("Dir::State::apt_mark_storage", "apt_mark_storage"); After reading the next commit, I've understood that "apt_mark_storage" is the default filename where to store the state of the auto-marks. 2. Although not important for us (and the behavior), the deletion of a special comment in apt/apt-pkg/depcache.cc is strange (and unexplained): @@ -665,7 +667,15 @@ void pkgDepCache::Update(PkgIterator const &Pkg) Update(P.ParentPkg().RevDependsList()); } - /*}}}*/ 3. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not consistent with how the code is indented nearby. (Although I don't like their mix of tabs and spaces.) commit ca9af74c6d592a4d908b48d8a7bc413de0dd694b Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Thu Oct 26 17:48:17 2017 +0300 Apt-mark: implement storing and retrieving list of automatically-installed packages Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old behavior: theoretically possible, because calls to read/writeStateFile() were injected into the old code (in the prev.commit) and they might contain an error/crash. However, practically, if there are no errors, no direct effect on the old behavior is expected because their return codes do not affect anything at the places where they are called. (Though, I'm not sure about the effect of things like: return _error->Error(_("Failed to replace file: %s"), AptMarkStorageFileName.c_str()); -- whther this would give a normal return code that is ignored or _error->Error() would somehow interrupt the normal execution? No, it won't interrupt the execution -- I've checked the sources for GlobalError::Error.) BTW, writeStateFile() should be a const method, why not? Another comment: This code uses the filename from the config parameter introduced in the prev.commit. A few things are not clear in the code: 1. What the value of delim_index is expected to be here? In the default value (from prev.commit) there is no '/'. AptMarkTempFileName = AptMarkStorageFileName.substr(0, delim_index + 1) + ".__tmp__apt_mark_storage"; 2. Why is iter not dereferenced here: linestr = iter.Name(); though its other previous uses are written with dereferencing, e.g.: StateCache const &stateCache = PkgState[iter->ID]; commit c5d71750cf00077f065b3bf7ea7875b5378f8761 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Oct 27 12:11:00 2017 +0300 Don't keep records for packages not installed Allow to toggle marking reinstalled packages as manually reinstalled. Based on ideas and code from apt-1.4.8 from Debian. Notes: quite clear, but: this seems to be 2 logically separate changes Effects on the old behavior: no (only errors in MarkAuto() might theoretically affect it) 1. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not consistent with how the code is indented nearby. (Although I don't like their mix of tabs and spaces, because it assumes that a tab is always 8 spaces...) I'd also have a look at the current indentation practice in the current Debian APT sources, and if they are more sensible I would probably re-indent according to their rules all our sources before introducing new changes. (In a hope to exchange some minor patches.) commit a8bdcec2777419949ded0dc3f668786b2aeeccb0 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Oct 27 16:34:17 2017 +0300 apt-get: implement handling of flag 'auto' for upgrade and dist-upgrade actions Fix apt-get install and dependency fixer to not mark packages manually installed when they're actually auto-installed. Based on ideas and code from apt-1.4.8 from Debian. Notes: 1. The parameter name "AptMarkInheritance" does not clarify whether the "auto" or "manual" mark is set on the condition named by the value of the parameter (like "at_least_one" etc.); I mean that something like "AptAutoMarkInheritance" would make it more clear for users looking at the configuration. Has this paramater name been copied from Debian? If so, then it's better not change it, probably, although it might be not very clear. TODO: review the rest of the code more thoroughly. 2. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not consistent with how the code is indented nearby. (Although I don't like their mix of tabs and spaces, because it assumes that a tab is always 8 spaces...) I'd also have a look at the current indentation practice in the current Debian APT sources, and if they are more sensible I would probably re-indent according to their rules all our sources before introducing new changes. (In a hope to exchange some minor patches.) commit 1371f9acfdeaf63c65baf6bc2911d0ab2136a931 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Oct 27 18:05:47 2017 +0300 apt-get: implement 'autoremove' action Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old behavior: no (it's a new command) Should I try to check the algorithm or the interested users will be testing it when using?.. 1. Although that's not very important for us, but I don't see a reason for the 2 deletions (in algorithms.cc, apt-get.cc): - /*}}}*/ commit d6e41a4773125ec65ec95362db2f430576aa7dd2 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Tue Oct 31 16:41:23 2017 +0300 Add option to provide debug information for "apt-get autoremove" virtual dependencies solving Based on ideas and code from apt-1.4.8 from Debian. Notes: quite clear Effects on the old behavior: no commit bc56eebab3f0249cdcae7bb4bb817a18cd336504 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Tue Oct 31 17:35:25 2017 +0300 Apt-shell: implement autoremove action Based on ideas and code from apt-1.4.8 from Debian. Notes: quite clear commit 345823fc49b9e9eb2bb6320de7c4767d3d1a1f16 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Thu Nov 2 11:05:44 2017 +0300 apt-get autoremove: improve dependencies calculation, ensure that versions and version ops are properly processed Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old behavior: no (This is just a change in the calculation algorithm for autoremove, which I have not been ready to reason about.) commit 346fee6fdb4803cc3a982a62498cb50c19de53eb Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Nov 3 10:44:28 2017 +0300 apt-shell: implement functions of apt-mark apt-shell: fix bug causing apt-shell to fail to start in case of warnings being produced. Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old bahavior: none (except for a fixed bug in apt-shell perhaps) If this is an independent bug, wouldn't it be better to fix it in a separate commit (so that it can be cherry-picked into other versions)? commit e61fb72b368b6e6b63b10f84adc2b433b9747a5a Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Dec 8 15:41:32 2017 +0300 Fix apt-mark auto/manual functions in apt-shell Notes: Effects on the old behavior: none commit d536c2da25a6d1cfeb16ec9519d5d5a41a8779e5 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Dec 8 15:45:39 2017 +0300 Use rpm header RPMTAG_AUTOINSTALLED for storing state of 'autoinstalled' flag Notes: Effects on the old behavior: can be either on APT, or on RPM (modifying the database). I haven't thoroughly studied this code. I've noticed an interesting thing for me: that the auto-flags are read/written in a package-by-package manner. Previously, I used to think that there is a way to get the data with one sweep. The writing is simply done in UpdateMarks() by iterating through the packages; and also immediately whenever rpm-install etc is done: see pkgRPMLibPM::AddToTransaction(). The reading happens in rpmListParser::Flags()--for a single package; this function is called in pkgcachegen.cc in pkgCacheGenerator::MergeList() during an iteration over a list of packages like this: NewPackage(Pkg,PackageName) ... Pkg->Flags = List.Flags(); ... whereas NewPackage() allocates a structure for the package and "registers" it in the cache etc. Also, an idea/question appeared to me: if this information is stored in the RPM db, there could be a way to change these flags not by means of APT, but rather through RPM's command-line interface (because these flags make sense without APT, for a substitute of APT; they are even stored in the common RPM db). Is there something like this? vseleznv@ said that there is apparently no way to do this through RPM now, but it has been discussed in RPM mailing lists. In a sense, of course, apt-mark is such a tool, but it has "apt" in its name (and uses APT libs)... commit 9f32c9202a062e9549341fb3c044268f601f9ac9 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Tue Dec 19 16:28:51 2017 +0300 0.5.15lorg2-alt59 - Implemented following actions and commands (closes: #34036): "apt-get autoremove", "apt-mark showmanual [package1 ...]", "apt-mark showauto [package1 ...]", "apt-mark manual package1 [package2 ...]", "apt-mark auto package1 [package2 ...]", "apt-mark showstate [package1 ...]". Notes: Some general thoughts on the whole series of changes. 1. It would be interesting to see a comparison of these changes in APT-RPM to the analogous code in Debian APT. It is not wanted to maintain a project which is further diverging. 2. For discussion, I'd group the changes into 3 groups: - Refactoring. - Setting the flags during the actions (such as install, dist-upgrade). - Autoremove algorithm and action, apt-mark additional tool. IMO, it's important that the "Setting the flags during the actions (such as install, dist-upgrade)" part works correctly, and--of course--that the refactoring leads to equivalent/better code. As for the autoremove algorithm (the really useful thing for the user), it can be easily modified or re-implemented on top of the flags if the users are not satisfied. (Perhaps, it's even possible experiment in Lua and make use of the auto-flags from Lua, isn't it?) The refactoring changes have not yet been checked thoroughly by me (I need some tools or comments to demonstrate they are OK). After my review, I believe the "Setting the flags during different actions (such as install, dist-upgrade)" does not break the old behavior. As for the interaction with RPM, I don't have a good knowledge of the RPM API to check it. 3. An idea came up to me after reading these changes: it concerns the "Setting the flags during different actions (such as install, dist-upgrade)" part, how to make it more clear and convincing. However, I would not like to suggest it if it diverges from Debian even more. To make sure that the decision how to change the flag was conciously made by the programmer in all cases, I'd add the value of the auto-flag as an obligatory parameter to all functions which create packages or modify their state, such as NewPackage(), MarkInstall() etc. and perhaps everything that modifies StateCache structure. (If the auto-flag value is explicit, then it is an explicit decision how to change the auto-flag on every kind of change and no cases are overlooked.)
(В ответ на комментарий №5) > Мои заметки (см. "Notes" у кажого коммита); самые общие -- на последнем > коммите. Там где-то написано, но ещё раз напишу, что трудно понимать, когда не > сильно связанные вещи меняются в одном коммите. Ещё интересно: что так же, как > в Debian? что исправлено/улучшено такого, что в Debian ещё не исправлено? > Сложно сравнивать с Debian, поскольку за много лет их версия apt-get была очень сильно изменена, теперь это проект, лишь местами похожий на тот apt-get, который сейчас в ALT. > $ git --no-pager log --reverse gears/sisyphus..autoremove > commit 5d970b58d5ec676ef543f678ab52d1921e3266b0 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Tue Oct 24 13:06:40 2017 +0300 > > Move some common functions out of apt-get.cc to library > > Replace legacy int types. > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > (TODO: review more thoroughly for new bugs.) Let's assume this is an > equivalent rewrite of the code. > > Effect on the old behavior: must be equivalent (not checked thoroughly). > > 1. Perhaps it would be easier to review if non-similar changes were > placed in separate commits (grouping the similar changes such as: > "Replace legacy int types.") > > 2. Was it necessary to make the changes in spacing (which can be > hidden with git diff -w)? If yes, it would be easier to review if they > were in a separate commit. > > commit 48ff0be3b7ada042e0b1cd33a2998c223117f8fa > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Thu Oct 26 15:58:22 2017 +0300 > > Add skeleton of apt-mark > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old behavior: This does not change the behavior of old > commands, because the calls to read/writeStateFile() do nothing as of > now. > > 1. I don't understand the comment and the following code yet: > > // In apt-get it's called 'Dir::State::apt_mark_storage', but format > used here is incompatible, due to that name is different > Cnf.CndSet("Dir::State::apt_mark_storage", "apt_mark_storage"); > > After reading the next commit, I've understood that "apt_mark_storage" > is the default filename where to store the state of the auto-marks. > I meant "Dir::State::extended_states" most likely instead of "Dir::State::apt_mark_storage" in the comment. This comment is definitely wrong. Apt-get from Debian uses file for similar purpose, but it has different format (it contains much more information which isn't needed for current case, like package description, etc), and due to that different name was chosen. > 2. Although not important for us (and the behavior), the deletion of a > special comment in apt/apt-pkg/depcache.cc is strange (and unexplained): > > @@ -665,7 +667,15 @@ void pkgDepCache::Update(PkgIterator const &Pkg) > Update(P.ParentPkg().RevDependsList()); > } > > - > /*}}}*/ > > 3. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > consistent with how the code is indented nearby. (Although I don't > like their mix of tabs and spaces.) > > commit ca9af74c6d592a4d908b48d8a7bc413de0dd694b > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Thu Oct 26 17:48:17 2017 +0300 > > Apt-mark: implement storing and retrieving list of automatically-installed > packages > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old behavior: theoretically possible, because calls to > read/writeStateFile() were injected into the old code (in the > prev.commit) and they might contain an error/crash. However, > practically, if there are no errors, no direct effect on the old > behavior is expected because their return codes do not affect anything > at the places where they are called. (Though, I'm not sure about the > effect of things like: > > return _error->Error(_("Failed to replace file: %s"), > AptMarkStorageFileName.c_str()); > > -- whther this would give a normal return code that is ignored or > _error->Error() would somehow interrupt the normal execution? No, it > won't interrupt the execution -- I've checked the sources for > GlobalError::Error.) > > BTW, writeStateFile() should be a const method, why not? > > Another comment: This code uses the filename from the config parameter > introduced in the prev.commit. A few things are not clear in the code: > > 1. What the value of delim_index is expected to be here? In the default > value (from prev.commit) there is no '/'. > > AptMarkTempFileName = AptMarkStorageFileName.substr(0, > delim_index + 1) + ".__tmp__apt_mark_storage"; > IIRC, "_config->FindFile("Dir::State::apt_mark_storage")", if not overridden in configuration, by default would return "/var/lib/apt/apt_mark_storage", thus by default, delim_index would be 12. Need to double check this. > 2. Why is iter not dereferenced here: > > linestr = iter.Name(); > > though its other previous uses are written with dereferencing, e.g.: > > StateCache const &stateCache = PkgState[iter->ID]; > This "iter" is an instance of class "pkgCache::PkgIterator", which is not actually a std::iterator. It has function "Name()", which returns package name, and it has "operator->", which returns pointer to another class "Package". It also has other functions. It's defined at cacheiterators.h. > commit c5d71750cf00077f065b3bf7ea7875b5378f8761 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Oct 27 12:11:00 2017 +0300 > > Don't keep records for packages not installed > > Allow to toggle marking reinstalled packages as manually reinstalled. > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > quite clear, but: this seems to be 2 logically separate changes > > Effects on the old behavior: no > (only errors in MarkAuto() might theoretically affect it) > > 1. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > consistent with how the code is indented nearby. (Although I don't > like their mix of tabs and spaces, because it assumes that a tab is > always 8 spaces...) > > I'd also have a look at the current indentation practice in the > current Debian APT sources, and if they are more sensible I would > probably re-indent according to their rules all our sources before > introducing new changes. (In a hope to exchange some minor patches.) > In case it's that big issue, instead of reformatting whole project, I'd prefer to go over all of my changes once more and reformat them according to current code style. > commit a8bdcec2777419949ded0dc3f668786b2aeeccb0 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Oct 27 16:34:17 2017 +0300 > > apt-get: implement handling of flag 'auto' for upgrade and dist-upgrade > actions > > Fix apt-get install and dependency fixer to not mark packages manually > installed when they're actually auto-installed. > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > 1. The parameter name "AptMarkInheritance" does not clarify whether the > "auto" or "manual" mark is set on the condition named by the value of > the parameter (like "at_least_one" etc.); I mean that something like > "AptAutoMarkInheritance" would make it more clear for users looking at > the configuration. Has this paramater name been copied from Debian? If > so, then it's better not change it, probably, although it might be not > very clear. > > TODO: review the rest of the code more thoroughly. > Yeah, good point, but I'd rename it to "AptMarkInheritanceAuto" in that case. No, it's not taken from Debian. > 2. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > consistent with how the code is indented nearby. (Although I don't > like their mix of tabs and spaces, because it assumes that a tab is > always 8 spaces...) > > I'd also have a look at the current indentation practice in the > current Debian APT sources, and if they are more sensible I would > probably re-indent according to their rules all our sources before > introducing new changes. (In a hope to exchange some minor patches.) > > commit 1371f9acfdeaf63c65baf6bc2911d0ab2136a931 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Oct 27 18:05:47 2017 +0300 > > apt-get: implement 'autoremove' action > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old behavior: no (it's a new command) > > Should I try to check the algorithm or the interested users will be > testing it when using?.. > I'm using it for more than half a year on my system, and system is still alive, but more testing is always welcome since I don't use autoremove too often. Test task #199955 for Sisyphus. > 1. Although that's not very important for us, but I don't see a reason > for the 2 deletions (in algorithms.cc, apt-get.cc): > > - > /*}}}*/ > > commit d6e41a4773125ec65ec95362db2f430576aa7dd2 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Tue Oct 31 16:41:23 2017 +0300 > > Add option to provide debug information for "apt-get autoremove" virtual > dependencies solving > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > quite clear > > Effects on the old behavior: no > > commit bc56eebab3f0249cdcae7bb4bb817a18cd336504 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Tue Oct 31 17:35:25 2017 +0300 > > Apt-shell: implement autoremove action > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > quite clear > > commit 345823fc49b9e9eb2bb6320de7c4767d3d1a1f16 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Thu Nov 2 11:05:44 2017 +0300 > > apt-get autoremove: improve dependencies calculation, ensure that versions > and version ops are properly processed > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old behavior: no > > (This is just a change in the calculation algorithm for autoremove, > which I have not been ready to reason about.) > When testing autoremove feature, I've found out that one of python packages was replaced by another one, and on next autoremove run, it was replaced back. It were packages "python-strict" and "python-relaxed", they both provide "python = 2.7", so it was ordered to uninstall since package named "python" is present in system. But there is a call to "Fix.Resolve(...)" at the end of autoremove function which plays role of additional sanity check that system dependencies are all satisfied, and this function proposed to install another package, i.e. if "python-strict" is removed by autoremove, "python-relaxed" is installed, and vice versa. And autoremove shouldn't install any packages, it clearly was a bug. But package "python" has different version, it provides "python = 2.7.14-alt7", and thus checking for specific provided version of virtual provide "python" (and any other virtual provide, if it has dependency on specific version; solution is generic) fixed this case. > commit 346fee6fdb4803cc3a982a62498cb50c19de53eb > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Nov 3 10:44:28 2017 +0300 > > apt-shell: implement functions of apt-mark > > apt-shell: fix bug causing apt-shell to fail to start in case of warnings > being produced. > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old bahavior: none (except for a fixed bug in apt-shell > perhaps) > > If this is an independent bug, wouldn't it be better to fix it in a > separate commit (so that it can be cherry-picked into other versions)? > You're right, but this bug was only discovered due to warning on file "apt_mark_storage" being missing (i.e. first run of new version of apt), and while bug is technically present in current version of apt, it's much harder to trigger it. Basically, you have to intentionally break something and get a warning. Still, it can probably be moved out to a separate, really tiny commit. > commit e61fb72b368b6e6b63b10f84adc2b433b9747a5a > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Dec 8 15:41:32 2017 +0300 > > Fix apt-mark auto/manual functions in apt-shell > > Notes: > Effects on the old behavior: none > > commit d536c2da25a6d1cfeb16ec9519d5d5a41a8779e5 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Dec 8 15:45:39 2017 +0300 > > Use rpm header RPMTAG_AUTOINSTALLED for storing state of 'autoinstalled' > flag > > Notes: > Effects on the old behavior: can be either on APT, or on RPM > (modifying the database). > > I haven't thoroughly studied this code. > > I've noticed an interesting thing for me: that the auto-flags are > read/written in a package-by-package manner. Previously, I used to > think that there is a way to get the data with one sweep. The writing > is simply done in UpdateMarks() by iterating through the packages; and > also immediately whenever rpm-install etc is done: see > pkgRPMLibPM::AddToTransaction(). The reading happens in > rpmListParser::Flags()--for a single package; this function is called > in pkgcachegen.cc in pkgCacheGenerator::MergeList() during an > iteration over a list of packages like this: > > NewPackage(Pkg,PackageName) > ... > Pkg->Flags = List.Flags(); > ... > > whereas NewPackage() allocates a structure for the package and > "registers" it in the cache etc. > > Also, an idea/question appeared to me: if this information is stored > in the RPM db, there could be a way to change these flags not by means > of APT, but rather through RPM's command-line interface (because these > flags make sense without APT, for a substitute of APT; they are even > stored in the common RPM db). Is there something like this? vseleznv@ > said that there is apparently no way to do this through RPM now, but > it has been discussed in RPM mailing lists. In a sense, of course, > apt-mark is such a tool, but it has "apt" in its name (and uses APT > libs)... > Since this information is now stored as a tag in package header, there's no API AFAIK to read this tag for every package at once, same goes for adding it. As for modifying the tag's value, you can see ugly approach (IIRC, proposed by upstream) at function "pkgRPMLibPM::UpdateMarks(...)". You can't just remove tag for a header stored at RPMDB, it'll go insane. Yes, currently there are no tools to change these tags via rpm. As mentioned above, it doesn't even have a proper API for changing tags. It's easy to implement a tool for just one specific tag (and thus one specific tag type and tag items count). Making a generic tool is a much bigger task, but would it really be needed? AFAIK, this is only tag that is getting changed without package reinstallation. As for non-generic tool, just for "autoinstalled" tag, the question is following: would upstream accept it? I don't think so. And for downstream solution apt-mark is not much different from rpm-mark. > commit 9f32c9202a062e9549341fb3c044268f601f9ac9 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Tue Dec 19 16:28:51 2017 +0300 > > 0.5.15lorg2-alt59 > > - Implemented following actions and commands (closes: #34036): > "apt-get autoremove", "apt-mark showmanual [package1 ...]", "apt-mark > showauto [package1 ...]", > "apt-mark manual package1 [package2 ...]", "apt-mark auto package1 > [package2 ...]", > "apt-mark showstate [package1 ...]". > > Notes: > Some general thoughts on the whole series of changes. > > 1. It would be interesting to see a comparison of these changes in > APT-RPM to the analogous code in Debian APT. It is not wanted to > maintain a project which is further diverging. > First commits with moving functions to common libraries are inspired by current state of apt-get from Debian, this allows to reduce code duplication, but autoremove algorithm is self-written, because the one from Debian required basically getting whole apt-get from Debian. Debian's version is integrated in their package processing, which has changed a lot compared to version from ALT. It's not a single function at apt-get from Debian, it's basically a part of most functions. I agree that diverging might be unwanted, but basically ALT is upstream of apt-rpm, which only looks like an old apt-get. > 2. For discussion, I'd group the changes into 3 groups: > > - Refactoring. > - Setting the flags during the actions (such as install, dist-upgrade). > - Autoremove algorithm and action, apt-mark additional tool. > > IMO, it's important that the "Setting the flags during the actions > (such as install, dist-upgrade)" part works correctly, and--of > course--that the refactoring leads to equivalent/better code. As for > the autoremove algorithm (the really useful thing for the user), it > can be easily modified or re-implemented on top of the flags if the > users are not satisfied. (Perhaps, it's even possible experiment in > Lua and make use of the auto-flags from Lua, isn't it?) > If Lua API allows to modify Flags of package and thus set/unset flag 'autoinstalled' before transaction is committed, it should work. But I didn't test it. > The refactoring changes have not yet been checked thoroughly by me (I > need some tools or comments to demonstrate they are OK). > > After my review, I believe the "Setting the flags during different > actions (such as install, dist-upgrade)" does not break the old > behavior. > > As for the interaction with RPM, I don't have a good knowledge of the > RPM API to check it. > > 3. An idea came up to me after reading these changes: it concerns the > "Setting the flags during different actions (such as install, > dist-upgrade)" part, how to make it more clear and convincing. > However, I would not like to suggest it if it diverges from > Debian even more. > > To make sure that the decision how to change the flag was conciously > made by the programmer in all cases, I'd add the value of the > auto-flag as an obligatory parameter to all functions which create > packages or modify their state, such as NewPackage(), MarkInstall() > etc. and perhaps everything that modifies StateCache structure. (If > the auto-flag value is explicit, then it is an explicit decision how > to change the auto-flag on every kind of change and no cases are > overlooked.) Yes, you're correct, additional modifications to apt-get's API can be made to include 'autoinstall' value. Just need an answer for the question whether API should be changed and diverged from Debian further.
(In reply to comment #6) > (В ответ на комментарий №5) > > Мои заметки (см. "Notes" у кажого коммита); самые общие -- на последнем > > коммите. Там где-то написано, но ещё раз напишу, что трудно понимать, когда не > > сильно связанные вещи меняются в одном коммите. Ещё интересно: что так же, как > > в Debian? что исправлено/улучшено такого, что в Debian ещё не исправлено? > > > > Сложно сравнивать с Debian, поскольку за много лет их версия apt-get была очень > сильно изменена, теперь это проект, лишь местами похожий на тот apt-get, > который сейчас в ALT. Наверное, это не самое важное (сравнение с Debian), но если ты неплохо разобрался в этих делах, то ради улучшения понимания участвующих в обсуждении не мог бы ты указать на конкретные функции в apt-rpm и в Debian (с тегом, коммитом) с небольшим комментарием, что разошлось. (Перечислять все -- это было бы долго, но хочется оттолкнуться от какого-нибудь небольшого комментария с твоим пониманием, чтобы начать знакомиться с расхождениями. Этот вопрос ещё и возник ниже здесь, когда обсуждаем стоит ли API менять ради явного обязательного выставления отметок auto при всех затрагивающих список пакетов операциях.) > > $ git --no-pager log --reverse gears/sisyphus..autoremove > > commit 5d970b58d5ec676ef543f678ab52d1921e3266b0 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Tue Oct 24 13:06:40 2017 +0300 > > > > Move some common functions out of apt-get.cc to library > > > > Replace legacy int types. > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > (TODO: review more thoroughly for new bugs.) Let's assume this is an > > equivalent rewrite of the code. > > > > Effect on the old behavior: must be equivalent (not checked thoroughly). > > > > 1. Perhaps it would be easier to review if non-similar changes were > > placed in separate commits (grouping the similar changes such as: > > "Replace legacy int types.") > > > > 2. Was it necessary to make the changes in spacing (which can be > > hidden with git diff -w)? If yes, it would be easier to review if they > > were in a separate commit. I see: spacing is discussed further below. > > commit 48ff0be3b7ada042e0b1cd33a2998c223117f8fa > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Thu Oct 26 15:58:22 2017 +0300 > > > > Add skeleton of apt-mark > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > Effects on the old behavior: This does not change the behavior of old > > commands, because the calls to read/writeStateFile() do nothing as of > > now. > > > > 1. I don't understand the comment and the following code yet: > > > > // In apt-get it's called 'Dir::State::apt_mark_storage', but format > > used here is incompatible, due to that name is different > > Cnf.CndSet("Dir::State::apt_mark_storage", "apt_mark_storage"); > > > > After reading the next commit, I've understood that "apt_mark_storage" > > is the default filename where to store the state of the auto-marks. > > > > I meant "Dir::State::extended_states" most likely instead of > "Dir::State::apt_mark_storage" in the comment. > This comment is definitely wrong. > Apt-get from Debian uses file for similar purpose, but it has different format > (it contains much more information which isn't needed for current case, like > package description, etc), > and due to that different name was chosen. Let's fix the comment in the commit then. It would help people reading this commit. > > 2. Although not important for us (and the behavior), the deletion of a > > special comment in apt/apt-pkg/depcache.cc is strange (and unexplained): > > > > @@ -665,7 +667,15 @@ void pkgDepCache::Update(PkgIterator const &Pkg) > > Update(P.ParentPkg().RevDependsList()); > > } > > > > - > > /*}}}*/ This is a minor tweak, but distracts the reader (if this is really an unintentional deletion, without any explanation). > > 3. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > > consistent with how the code is indented nearby. (Although I don't > > like their mix of tabs and spaces.) > > > > commit ca9af74c6d592a4d908b48d8a7bc413de0dd694b > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Thu Oct 26 17:48:17 2017 +0300 > > > > Apt-mark: implement storing and retrieving list of automatically-installed > > packages > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > Effects on the old behavior: theoretically possible, because calls to > > read/writeStateFile() were injected into the old code (in the > > prev.commit) and they might contain an error/crash. However, > > practically, if there are no errors, no direct effect on the old > > behavior is expected because their return codes do not affect anything > > at the places where they are called. (Though, I'm not sure about the > > effect of things like: > > > > return _error->Error(_("Failed to replace file: %s"), > > AptMarkStorageFileName.c_str()); > > > > -- whther this would give a normal return code that is ignored or > > _error->Error() would somehow interrupt the normal execution? No, it > > won't interrupt the execution -- I've checked the sources for > > GlobalError::Error.) > > > > BTW, writeStateFile() should be a const method, why not? > > > > Another comment: This code uses the filename from the config parameter > > introduced in the prev.commit. A few things are not clear in the code: > > > > 1. What the value of delim_index is expected to be here? In the default > > value (from prev.commit) there is no '/'. > > > > AptMarkTempFileName = AptMarkStorageFileName.substr(0, > > delim_index + 1) + ".__tmp__apt_mark_storage"; > > > > IIRC, "_config->FindFile("Dir::State::apt_mark_storage")", if not overridden in > configuration, by default would return "/var/lib/apt/apt_mark_storage", > thus by default, delim_index would be 12. Need to double check this. Thanks for the explanation! I'll come back to this code and re-read it. > > 2. Why is iter not dereferenced here: > > > > linestr = iter.Name(); > > > > though its other previous uses are written with dereferencing, e.g.: > > > > StateCache const &stateCache = PkgState[iter->ID]; > > > > This "iter" is an instance of class "pkgCache::PkgIterator", which is not > actually a std::iterator. > It has function "Name()", which returns package name, and it has "operator->", > which returns pointer to another class "Package". > It also has other functions. It's defined at cacheiterators.h. Thanks for the explanation! > > commit c5d71750cf00077f065b3bf7ea7875b5378f8761 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Fri Oct 27 12:11:00 2017 +0300 > > > > Don't keep records for packages not installed > > > > Allow to toggle marking reinstalled packages as manually reinstalled. > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > quite clear, but: this seems to be 2 logically separate changes > > > > Effects on the old behavior: no > > (only errors in MarkAuto() might theoretically affect it) > > > > 1. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > > consistent with how the code is indented nearby. (Although I don't > > like their mix of tabs and spaces, because it assumes that a tab is > > always 8 spaces...) > > > > I'd also have a look at the current indentation practice in the > > current Debian APT sources, and if they are more sensible I would > > probably re-indent according to their rules all our sources before > > introducing new changes. (In a hope to exchange some minor patches.) > > > > In case it's that big issue, instead of reformatting whole project, I'd prefer > to go over all of my changes once more and reformat them according to current > code style. Yes, that would be useful. Additional differences without any additional meaning (and formatting inconsistencies) make reading the commits more difficult. I'd suggest (if desired) to make formatting changes in a separate commit always (to separate them from meaningful differences), and in the meaningful commits to follow the surrounding style. > > commit a8bdcec2777419949ded0dc3f668786b2aeeccb0 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Fri Oct 27 16:34:17 2017 +0300 > > > > apt-get: implement handling of flag 'auto' for upgrade and dist-upgrade > > actions > > > > Fix apt-get install and dependency fixer to not mark packages manually > > installed when they're actually auto-installed. > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > 1. The parameter name "AptMarkInheritance" does not clarify whether the > > "auto" or "manual" mark is set on the condition named by the value of > > the parameter (like "at_least_one" etc.); I mean that something like > > "AptAutoMarkInheritance" would make it more clear for users looking at > > the configuration. Has this paramater name been copied from Debian? If > > so, then it's better not change it, probably, although it might be not > > very clear. > > > > TODO: review the rest of the code more thoroughly. > > > > Yeah, good point, but I'd rename it to "AptMarkInheritanceAuto" in that case. > No, it's not taken from Debian. Yes, then let's rename it. As for the name: why is your name better? How do you understand it? In English, the main word is usually the last one (so this value is about inheritance, and I'd expect "inheritance" to be the main word); and modifiers are placed before the main word ("green car", "package manager") or by means of "of" after it ("manager of packages"). That's why I didn't suggest putting "Auto" at the end. What's your understanding of the suggested name? > > 2. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > > consistent with how the code is indented nearby. (Although I don't > > like their mix of tabs and spaces, because it assumes that a tab is > > always 8 spaces...) > > > > I'd also have a look at the current indentation practice in the > > current Debian APT sources, and if they are more sensible I would > > probably re-indent according to their rules all our sources before > > introducing new changes. (In a hope to exchange some minor patches.) > > > > commit 1371f9acfdeaf63c65baf6bc2911d0ab2136a931 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Fri Oct 27 18:05:47 2017 +0300 > > > > apt-get: implement 'autoremove' action > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > Effects on the old behavior: no (it's a new command) > > > > Should I try to check the algorithm or the interested users will be > > testing it when using?.. > > > > I'm using it for more than half a year on my system, and system is still alive, > but more testing is always welcome since I don't use autoremove too often. > Test task #199955 for Sisyphus. Thanks! I believe that your testing and the testing that will be done after it is committed to Sisyphus by all the users is enough. > > 1. Although that's not very important for us, but I don't see a reason > > for the 2 deletions (in algorithms.cc, apt-get.cc): > > > > - > > /*}}}*/ Once again, if there is no reason for this deletion, let's leave it there as in the original code. (Or delete it in separate commit if you don't like this line. This seems to be unrelated to the main change in this commit.) > > commit d6e41a4773125ec65ec95362db2f430576aa7dd2 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Tue Oct 31 16:41:23 2017 +0300 > > > > Add option to provide debug information for "apt-get autoremove" virtual > > dependencies solving > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > quite clear > > > > Effects on the old behavior: no > > > > commit bc56eebab3f0249cdcae7bb4bb817a18cd336504 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Tue Oct 31 17:35:25 2017 +0300 > > > > Apt-shell: implement autoremove action > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > quite clear > > > > commit 345823fc49b9e9eb2bb6320de7c4767d3d1a1f16 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Thu Nov 2 11:05:44 2017 +0300 > > > > apt-get autoremove: improve dependencies calculation, ensure that versions > > and version ops are properly processed > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > Effects on the old behavior: no > > > > (This is just a change in the calculation algorithm for autoremove, > > which I have not been ready to reason about.) > > > > When testing autoremove feature, I've found out that one of python packages was > replaced by another one, and on next autoremove run, it was replaced back. > It were packages "python-strict" and "python-relaxed", they both provide > "python = 2.7", so it was ordered to uninstall since package named "python" is > present in system. > But there is a call to "Fix.Resolve(...)" at the end of autoremove function > which plays role of additional sanity check that system dependencies are all > satisfied, > and this function proposed to install another package, i.e. if "python-strict" > is removed by autoremove, "python-relaxed" is installed, and vice versa. And > autoremove shouldn't install any packages, it clearly was a bug. > But package "python" has different version, it provides "python = 2.7.14-alt7", > and thus checking for specific provided version of virtual provide "python" > (and any other virtual provide, if it has dependency on specific version; > solution is generic) fixed this case. Thanks for your explanation! I'll come back and re-read this change. Let's put this explanation into a comment or a commit message. > > commit 346fee6fdb4803cc3a982a62498cb50c19de53eb > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Fri Nov 3 10:44:28 2017 +0300 > > > > apt-shell: implement functions of apt-mark > > > > apt-shell: fix bug causing apt-shell to fail to start in case of warnings > > being produced. > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > Effects on the old bahavior: none (except for a fixed bug in apt-shell > > perhaps) > > > > If this is an independent bug, wouldn't it be better to fix it in a > > separate commit (so that it can be cherry-picked into other versions)? > > > > You're right, but this bug was only discovered due to warning on file > "apt_mark_storage" being missing (i.e. first run of new version of apt), > and while bug is technically present in current version of apt, it's much > harder to trigger it. Basically, you have to intentionally break something and > get a warning. Still, it can probably be moved out to a separate, really tiny > commit. I believe that a separate tiny commit would be better anyway (even if the bug is difficult to trigger in the original code.) Let's make a separate tiny commit. > > commit e61fb72b368b6e6b63b10f84adc2b433b9747a5a > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Fri Dec 8 15:41:32 2017 +0300 > > > > Fix apt-mark auto/manual functions in apt-shell > > > > Notes: > > Effects on the old behavior: none > > > > commit d536c2da25a6d1cfeb16ec9519d5d5a41a8779e5 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Fri Dec 8 15:45:39 2017 +0300 > > > > Use rpm header RPMTAG_AUTOINSTALLED for storing state of 'autoinstalled' > > flag > > > > Notes: > > Effects on the old behavior: can be either on APT, or on RPM > > (modifying the database). > > > > I haven't thoroughly studied this code. > > > > I've noticed an interesting thing for me: that the auto-flags are > > read/written in a package-by-package manner. Previously, I used to > > think that there is a way to get the data with one sweep. The writing > > is simply done in UpdateMarks() by iterating through the packages; and > > also immediately whenever rpm-install etc is done: see > > pkgRPMLibPM::AddToTransaction(). The reading happens in > > rpmListParser::Flags()--for a single package; this function is called > > in pkgcachegen.cc in pkgCacheGenerator::MergeList() during an > > iteration over a list of packages like this: > > > > NewPackage(Pkg,PackageName) > > ... > > Pkg->Flags = List.Flags(); > > ... > > > > whereas NewPackage() allocates a structure for the package and > > "registers" it in the cache etc. > > > > Also, an idea/question appeared to me: if this information is stored > > in the RPM db, there could be a way to change these flags not by means > > of APT, but rather through RPM's command-line interface (because these > > flags make sense without APT, for a substitute of APT; they are even > > stored in the common RPM db). Is there something like this? vseleznv@ > > said that there is apparently no way to do this through RPM now, but > > it has been discussed in RPM mailing lists. In a sense, of course, > > apt-mark is such a tool, but it has "apt" in its name (and uses APT > > libs)... > > > > Since this information is now stored as a tag in package header, there's no API > AFAIK to read this tag for every package at once, > same goes for adding it. As for modifying the tag's value, you can see ugly > approach (IIRC, proposed by upstream) at function > "pkgRPMLibPM::UpdateMarks(...)". You can't just remove tag for a header stored > at RPMDB, it'll go insane. Thanks for this explanation! > Yes, currently there are no tools to change these tags via rpm. As mentioned > above, it doesn't even have a proper API for changing tags. > It's easy to implement a tool for just one specific tag (and thus one specific > tag type and tag items count). > Making a generic tool is a much bigger task, but would it really be needed? > AFAIK, this is only tag that is getting changed without package reinstallation. > As for non-generic tool, just for "autoinstalled" tag, the question is > following: would upstream accept it? I don't think so. And for downstream > solution apt-mark is not much different from rpm-mark. I agree with you. It's not a part of the current task to bother with a special rpm-only tool for this. I have just noted this, so that vseleznv@ and others could consider this improvement to rpm. > > commit 9f32c9202a062e9549341fb3c044268f601f9ac9 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Tue Dec 19 16:28:51 2017 +0300 > > > > 0.5.15lorg2-alt59 > > > > - Implemented following actions and commands (closes: #34036): > > "apt-get autoremove", "apt-mark showmanual [package1 ...]", "apt-mark > > showauto [package1 ...]", > > "apt-mark manual package1 [package2 ...]", "apt-mark auto package1 > > [package2 ...]", > > "apt-mark showstate [package1 ...]". > > > > Notes: > > Some general thoughts on the whole series of changes. > > > > 1. It would be interesting to see a comparison of these changes in > > APT-RPM to the analogous code in Debian APT. It is not wanted to > > maintain a project which is further diverging. > > > > First commits with moving functions to common libraries are inspired by current > state of apt-get from Debian, this allows to reduce code duplication, To check them, I'm about to repeat your movements myself and then have a look at the diff between my and your results. (To see whether we get equivalent code.) No magic tools are unfortunately known to us that would help here. In general, it's a problem: how the author of such changes should present them to prove to the reader that the new code is equivalent to the old code... > but autoremove algorithm is self-written, because the one from Debian required > basically getting whole apt-get from Debian. > Debian's version is integrated in their package processing, which has changed a > lot compared to version from ALT. > It's not a single function at apt-get from Debian, it's basically a part of > most functions. > > I agree that diverging might be unwanted, but basically ALT is upstream of > apt-rpm, which only looks like an old apt-get. > > > 2. For discussion, I'd group the changes into 3 groups: > > > > - Refactoring. > > - Setting the flags during the actions (such as install, dist-upgrade). > > - Autoremove algorithm and action, apt-mark additional tool. > > > > IMO, it's important that the "Setting the flags during the actions > > (such as install, dist-upgrade)" part works correctly, and--of > > course--that the refactoring leads to equivalent/better code. As for > > the autoremove algorithm (the really useful thing for the user), it > > can be easily modified or re-implemented on top of the flags if the > > users are not satisfied. (Perhaps, it's even possible experiment in > > Lua and make use of the auto-flags from Lua, isn't it?) > > > > If Lua API allows to modify Flags of package and thus set/unset flag > 'autoinstalled' before transaction is committed, > it should work. But I didn't test it. I see. That's not a part of the current task, IMO. > > The refactoring changes have not yet been checked thoroughly by me (I > > need some tools or comments to demonstrate they are OK). > > > > After my review, I believe the "Setting the flags during different > > actions (such as install, dist-upgrade)" does not break the old > > behavior. > > > > As for the interaction with RPM, I don't have a good knowledge of the > > RPM API to check it. > > > > 3. An idea came up to me after reading these changes: it concerns the > > "Setting the flags during different actions (such as install, > > dist-upgrade)" part, how to make it more clear and convincing. > > However, I would not like to suggest it if it diverges from > > Debian even more. > > > > To make sure that the decision how to change the flag was conciously > > made by the programmer in all cases, I'd add the value of the > > auto-flag as an obligatory parameter to all functions which create > > packages or modify their state, such as NewPackage(), MarkInstall() > > etc. and perhaps everything that modifies StateCache structure. (If > > the auto-flag value is explicit, then it is an explicit decision how > > to change the auto-flag on every kind of change and no cases are > > overlooked.) > > Yes, you're correct, additional modifications to apt-get's API can be made to > include 'autoinstall' value. > Just need an answer for the question whether API should be changed and diverged > from Debian further. Well, if we agree that such a rewrite would make us (and other people who read the code) more confident that no case where we need to decide on the value of the auto-mark has been left unnoticed, then it is worth it (IMO). As for the divergence from Debian, it is big already, isn't it? Or is there a hope that the API is compatible now, so that some code can be transferred? But if that code does some work with modifying the lists of packages, then it will need to be adapted to work with our auto-marks. (Otherwise it could leave wrong marks after its work.) So, it seems that API compatibility is not important in this case (in things related to modifying the lists of packages and deciding which mark to set), perhaps even harmful (decisions about the auto-marks could be left out because the current API doesn't make them obligatory).
On my part, I see 2 things to do now: 1. Check the relocations of functions between files by repeating these changes myself. 2. Perhaps (when I'm done with 1), suggest an API change which would make the decision about auto-marks obligatory arguments of functions which modify the lists of packages. (But darktemplar@ or soemone else could try this, too, I don't insist on being the one who does this.)
(In reply to comment #7) > > > $ git --no-pager log --reverse gears/sisyphus..autoremove > > > commit 5d970b58d5ec676ef543f678ab52d1921e3266b0 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Tue Oct 24 13:06:40 2017 +0300 > > > > > > Move some common functions out of apt-get.cc to library > > > > > > Replace legacy int types. > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > (TODO: review more thoroughly for new bugs.) Let's assume this is an > > > equivalent rewrite of the code. > > > > > > Effect on the old behavior: must be equivalent (not checked thoroughly). > > > > > > 1. Perhaps it would be easier to review if non-similar changes were > > > placed in separate commits (grouping the similar changes such as: > > > "Replace legacy int types.") > > > > > > 2. Was it necessary to make the changes in spacing (which can be > > > hidden with git diff -w)? If yes, it would be easier to review if they > > > were in a separate commit. (I believe that we don't want to compare the code to Debian at the first place. So, then we want it to compare to our old code.) So, to make it easier to compare with our old code, could you please split this commit so that there are separate commits (in any order you like and any number of individual commits): 1. with the relocations and minimal necessary changes (due to the relocations), so that git show -w --color-moved 5d970b58d5ec676ef543f678ab52d1921e3266b0 would show all lines as moved (I see them at the new locations in yellow and cyan; at the old locations -- in magenta); 2. with the code improvements (so that "git show" shows no additional noise). > I see: spacing is discussed further below. ... > > > commit c5d71750cf00077f065b3bf7ea7875b5378f8761 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Fri Oct 27 12:11:00 2017 +0300 > > > > > > Don't keep records for packages not installed > > > > > > Allow to toggle marking reinstalled packages as manually reinstalled. > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > quite clear, but: this seems to be 2 logically separate changes > > > > > > Effects on the old behavior: no > > > (only errors in MarkAuto() might theoretically affect it) > > > > > > 1. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > > > consistent with how the code is indented nearby. (Although I don't > > > like their mix of tabs and spaces, because it assumes that a tab is > > > always 8 spaces...) > > > > > > I'd also have a look at the current indentation practice in the > > > current Debian APT sources, and if they are more sensible I would > > > probably re-indent according to their rules all our sources before > > > introducing new changes. (In a hope to exchange some minor patches.) > > > > > > > In case it's that big issue, instead of reformatting whole project, I'd prefer > > to go over all of my changes once more and reformat them according to current > > code style. > > Yes, that would be useful. Additional differences without any additional > meaning (and formatting inconsistencies) make reading the commits more > difficult. > > I'd suggest (if desired) to make formatting changes in a separate commit always > (to separate them from meaningful differences), and in the meaningful commits > to follow the surrounding style. ... > > > Notes: > > > Some general thoughts on the whole series of changes. > > > > > > 1. It would be interesting to see a comparison of these changes in > > > APT-RPM to the analogous code in Debian APT. It is not wanted to > > > maintain a project which is further diverging. > > > > > > > First commits with moving functions to common libraries are inspired by current > > state of apt-get from Debian, this allows to reduce code duplication, > > To check them, I'm about to repeat your movements myself and then have a look > at the diff between my and your results. (To see whether we get equivalent > code.) No magic tools are unfortunately known to us that would help here. > > In general, it's a problem: how the author of such changes should present them > to prove to the reader that the new code is equivalent to the old code... I've found out that the role of such a "magic tool" for us can be performed by Git >= 2.15, namely git diff --color-moved (thanks to https://stackoverflow.com/a/47193017/94687 ). * * * As a sidenote, I'll write down some information about the tools which I know so that people including me can find this information after a while. Before I learned about git diff --color-moved, I had some thoughts of: 1) making use of Coccinelle+Herodotos, which I remembered to have the ability to analyze C projects (including the kernel). But it is more suitable for tracking certain patterns of code rather than for tracking whole chunks of code as we needed. http://coccinelle.lip6.fr/papers/aosd10.pdf 2) writing a simple comparison tool based on the language-c Haskell library, which does the parsing and some initial semantic analysis, i.e. the binding of variable names according to the scope. 3) looking into Rietveld/Gerrit, which I was told to have been written by Guido van Rossum to facilitate code review. But it doesn't seem to advertise such a feature like tracking moved code.
(В ответ на комментарий №7) > (In reply to comment #6) > > (В ответ на комментарий №5) > > > Мои заметки (см. "Notes" у кажого коммита); самые общие -- на последнем > > > коммите. Там где-то написано, но ещё раз напишу, что трудно понимать, когда не > > > сильно связанные вещи меняются в одном коммите. Ещё интересно: что так же, как > > > в Debian? что исправлено/улучшено такого, что в Debian ещё не исправлено? > > > > > > > Сложно сравнивать с Debian, поскольку за много лет их версия apt-get была очень > > сильно изменена, теперь это проект, лишь местами похожий на тот apt-get, > > который сейчас в ALT. > > Наверное, это не самое важное (сравнение с Debian), но если ты неплохо > разобрался в этих делах, то ради улучшения понимания участвующих в обсуждении > не мог бы ты указать на конкретные функции в apt-rpm и в Debian (с тегом, > коммитом) с небольшим комментарием, что разошлось. (Перечислять все -- это было > бы долго, но хочется оттолкнуться от какого-нибудь небольшого комментария с > твоим пониманием, чтобы начать знакомиться с расхождениями. Этот вопрос ещё и > возник ниже здесь, когда обсуждаем стоит ли API менять ради явного > обязательного выставления отметок auto при всех затрагивающих список пакетов > операциях.) > Не до конца понятен вопрос - он про разницу в реализации autoremove или про разницу в apt вообще? Самая большая разница в apt, которая сразу приходит на ум, это поддержка multiarch в новом apt. Также, как я уже упоминал, apt-get из Debian хранит немало информации в специальном файле "Dir::State::extended_states", в то время как apt-get из ALT не хранит в данный момент дополнительной информации об установленных пакетах где-либо за пределами rpmdb. Если же говорить про реализацию autoremove, то в apt-get из Debian логика autoremove размазана по всему коду в виде различных оценок (score) и отметок (mark), которые в том числе как минимум частично берутся из уже указанного файла "Dir::State::extended_states". В моей реализации за autoremove отвечают несколько отдельных функций. К сожалению, на конкретные коммиты тут сложно дать ссылки. > > > $ git --no-pager log --reverse gears/sisyphus..autoremove > > > commit 5d970b58d5ec676ef543f678ab52d1921e3266b0 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Tue Oct 24 13:06:40 2017 +0300 > > > > > > Move some common functions out of apt-get.cc to library > > > > > > Replace legacy int types. > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > (TODO: review more thoroughly for new bugs.) Let's assume this is an > > > equivalent rewrite of the code. > > > > > > Effect on the old behavior: must be equivalent (not checked thoroughly). > > > > > > 1. Perhaps it would be easier to review if non-similar changes were > > > placed in separate commits (grouping the similar changes such as: > > > "Replace legacy int types.") > > > > > > 2. Was it necessary to make the changes in spacing (which can be > > > hidden with git diff -w)? If yes, it would be easier to review if they > > > were in a separate commit. > > I see: spacing is discussed further below. > > > > commit 48ff0be3b7ada042e0b1cd33a2998c223117f8fa > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Thu Oct 26 15:58:22 2017 +0300 > > > > > > Add skeleton of apt-mark > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > Effects on the old behavior: This does not change the behavior of old > > > commands, because the calls to read/writeStateFile() do nothing as of > > > now. > > > > > > 1. I don't understand the comment and the following code yet: > > > > > > // In apt-get it's called 'Dir::State::apt_mark_storage', but format > > > used here is incompatible, due to that name is different > > > Cnf.CndSet("Dir::State::apt_mark_storage", "apt_mark_storage"); > > > > > > After reading the next commit, I've understood that "apt_mark_storage" > > > is the default filename where to store the state of the auto-marks. > > > > > > > I meant "Dir::State::extended_states" most likely instead of > > "Dir::State::apt_mark_storage" in the comment. > > This comment is definitely wrong. > > Apt-get from Debian uses file for similar purpose, but it has different format > > (it contains much more information which isn't needed for current case, like > > package description, etc), > > and due to that different name was chosen. > > Let's fix the comment in the commit then. It would help people reading this > commit. > Sure, I'll fix it. > > > 2. Although not important for us (and the behavior), the deletion of a > > > special comment in apt/apt-pkg/depcache.cc is strange (and unexplained): > > > > > > @@ -665,7 +667,15 @@ void pkgDepCache::Update(PkgIterator const &Pkg) > > > Update(P.ParentPkg().RevDependsList()); > > > } > > > > > > - > > > /*}}}*/ > > This is a minor tweak, but distracts the reader (if this is really an > unintentional deletion, without any explanation). > I'll fix this too. > > > 3. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > > > consistent with how the code is indented nearby. (Although I don't > > > like their mix of tabs and spaces.) > > > > > > commit ca9af74c6d592a4d908b48d8a7bc413de0dd694b > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Thu Oct 26 17:48:17 2017 +0300 > > > > > > Apt-mark: implement storing and retrieving list of automatically-installed > > > packages > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > Effects on the old behavior: theoretically possible, because calls to > > > read/writeStateFile() were injected into the old code (in the > > > prev.commit) and they might contain an error/crash. However, > > > practically, if there are no errors, no direct effect on the old > > > behavior is expected because their return codes do not affect anything > > > at the places where they are called. (Though, I'm not sure about the > > > effect of things like: > > > > > > return _error->Error(_("Failed to replace file: %s"), > > > AptMarkStorageFileName.c_str()); > > > > > > -- whther this would give a normal return code that is ignored or > > > _error->Error() would somehow interrupt the normal execution? No, it > > > won't interrupt the execution -- I've checked the sources for > > > GlobalError::Error.) > > > > > > BTW, writeStateFile() should be a const method, why not? > > > > > > Another comment: This code uses the filename from the config parameter > > > introduced in the prev.commit. A few things are not clear in the code: > > > > > > 1. What the value of delim_index is expected to be here? In the default > > > value (from prev.commit) there is no '/'. > > > > > > AptMarkTempFileName = AptMarkStorageFileName.substr(0, > > > delim_index + 1) + ".__tmp__apt_mark_storage"; > > > > > > > IIRC, "_config->FindFile("Dir::State::apt_mark_storage")", if not overridden in > > configuration, by default would return "/var/lib/apt/apt_mark_storage", > > thus by default, delim_index would be 12. Need to double check this. > > Thanks for the explanation! I'll come back to this code and re-read it. > > > > 2. Why is iter not dereferenced here: > > > > > > linestr = iter.Name(); > > > > > > though its other previous uses are written with dereferencing, e.g.: > > > > > > StateCache const &stateCache = PkgState[iter->ID]; > > > > > > > This "iter" is an instance of class "pkgCache::PkgIterator", which is not > > actually a std::iterator. > > It has function "Name()", which returns package name, and it has "operator->", > > which returns pointer to another class "Package". > > It also has other functions. It's defined at cacheiterators.h. > > Thanks for the explanation! > > > > commit c5d71750cf00077f065b3bf7ea7875b5378f8761 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Fri Oct 27 12:11:00 2017 +0300 > > > > > > Don't keep records for packages not installed > > > > > > Allow to toggle marking reinstalled packages as manually reinstalled. > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > quite clear, but: this seems to be 2 logically separate changes > > > > > > Effects on the old behavior: no > > > (only errors in MarkAuto() might theoretically affect it) > > > > > > 1. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > > > consistent with how the code is indented nearby. (Although I don't > > > like their mix of tabs and spaces, because it assumes that a tab is > > > always 8 spaces...) > > > > > > I'd also have a look at the current indentation practice in the > > > current Debian APT sources, and if they are more sensible I would > > > probably re-indent according to their rules all our sources before > > > introducing new changes. (In a hope to exchange some minor patches.) > > > > > > > In case it's that big issue, instead of reformatting whole project, I'd prefer > > to go over all of my changes once more and reformat them according to current > > code style. > > Yes, that would be useful. Additional differences without any additional > meaning (and formatting inconsistencies) make reading the commits more > difficult. > > I'd suggest (if desired) to make formatting changes in a separate commit always > (to separate them from meaningful differences), and in the meaningful commits > to follow the surrounding style. > Ok, I'll work on this. > > > commit a8bdcec2777419949ded0dc3f668786b2aeeccb0 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Fri Oct 27 16:34:17 2017 +0300 > > > > > > apt-get: implement handling of flag 'auto' for upgrade and dist-upgrade > > > actions > > > > > > Fix apt-get install and dependency fixer to not mark packages manually > > > installed when they're actually auto-installed. > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > 1. The parameter name "AptMarkInheritance" does not clarify whether the > > > "auto" or "manual" mark is set on the condition named by the value of > > > the parameter (like "at_least_one" etc.); I mean that something like > > > "AptAutoMarkInheritance" would make it more clear for users looking at > > > the configuration. Has this paramater name been copied from Debian? If > > > so, then it's better not change it, probably, although it might be not > > > very clear. > > > > > > TODO: review the rest of the code more thoroughly. > > > > > > > Yeah, good point, but I'd rename it to "AptMarkInheritanceAuto" in that case. > > No, it's not taken from Debian. > > Yes, then let's rename it. As for the name: why is your name better? How do you > understand it? > > In English, the main word is usually the last one (so this value is about > inheritance, and I'd expect "inheritance" to be the main word); and modifiers > are placed before the main word ("green car", "package manager") or by means of > "of" after it ("manager of packages"). That's why I didn't suggest putting > "Auto" at the end. What's your understanding of the suggested name? > Well, I'd read it as "obsoleting packages are marked auto" when "specific condition is met". First quoted sentence is a name of variable. I want to emphasize 'auto'. When condition is met, obsoleting package is marked 'auto' instead of 'manual'. Thus main word is 'auto', not 'inheritance'. And 'specific condition' is stored as value of that variable. > > > 2. The spacing (with tabs) in apt/apt-pkg/rpm/rpmsystem.cc is not > > > consistent with how the code is indented nearby. (Although I don't > > > like their mix of tabs and spaces, because it assumes that a tab is > > > always 8 spaces...) > > > > > > I'd also have a look at the current indentation practice in the > > > current Debian APT sources, and if they are more sensible I would > > > probably re-indent according to their rules all our sources before > > > introducing new changes. (In a hope to exchange some minor patches.) > > > > > > commit 1371f9acfdeaf63c65baf6bc2911d0ab2136a931 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Fri Oct 27 18:05:47 2017 +0300 > > > > > > apt-get: implement 'autoremove' action > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > Effects on the old behavior: no (it's a new command) > > > > > > Should I try to check the algorithm or the interested users will be > > > testing it when using?.. > > > > > > > I'm using it for more than half a year on my system, and system is still alive, > > but more testing is always welcome since I don't use autoremove too often. > > Test task #199955 for Sisyphus. > > Thanks! I believe that your testing and the testing that will be done after it > is committed to Sisyphus by all the users is enough. > > > > 1. Although that's not very important for us, but I don't see a reason > > > for the 2 deletions (in algorithms.cc, apt-get.cc): > > > > > > - > > > /*}}}*/ > > Once again, if there is no reason for this deletion, let's leave it there as in > the original code. (Or delete it in separate commit if you don't like this > line. This seems to be unrelated to the main change in this commit.) > I'll fix this unneeded deletion. > > > commit d6e41a4773125ec65ec95362db2f430576aa7dd2 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Tue Oct 31 16:41:23 2017 +0300 > > > > > > Add option to provide debug information for "apt-get autoremove" virtual > > > dependencies solving > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > quite clear > > > > > > Effects on the old behavior: no > > > > > > commit bc56eebab3f0249cdcae7bb4bb817a18cd336504 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Tue Oct 31 17:35:25 2017 +0300 > > > > > > Apt-shell: implement autoremove action > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > quite clear > > > > > > commit 345823fc49b9e9eb2bb6320de7c4767d3d1a1f16 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Thu Nov 2 11:05:44 2017 +0300 > > > > > > apt-get autoremove: improve dependencies calculation, ensure that versions > > > and version ops are properly processed > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > Effects on the old behavior: no > > > > > > (This is just a change in the calculation algorithm for autoremove, > > > which I have not been ready to reason about.) > > > > > > > When testing autoremove feature, I've found out that one of python packages was > > replaced by another one, and on next autoremove run, it was replaced back. > > It were packages "python-strict" and "python-relaxed", they both provide > > "python = 2.7", so it was ordered to uninstall since package named "python" is > > present in system. > > But there is a call to "Fix.Resolve(...)" at the end of autoremove function > > which plays role of additional sanity check that system dependencies are all > > satisfied, > > and this function proposed to install another package, i.e. if "python-strict" > > is removed by autoremove, "python-relaxed" is installed, and vice versa. And > > autoremove shouldn't install any packages, it clearly was a bug. > > But package "python" has different version, it provides "python = 2.7.14-alt7", > > and thus checking for specific provided version of virtual provide "python" > > (and any other virtual provide, if it has dependency on specific version; > > solution is generic) fixed this case. > > Thanks for your explanation! I'll come back and re-read this change. Let's put > this explanation into a comment or a commit message. > Sure, I'll do this. > > > commit 346fee6fdb4803cc3a982a62498cb50c19de53eb > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Fri Nov 3 10:44:28 2017 +0300 > > > > > > apt-shell: implement functions of apt-mark > > > > > > apt-shell: fix bug causing apt-shell to fail to start in case of warnings > > > being produced. > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > Notes: > > > Effects on the old bahavior: none (except for a fixed bug in apt-shell > > > perhaps) > > > > > > If this is an independent bug, wouldn't it be better to fix it in a > > > separate commit (so that it can be cherry-picked into other versions)? > > > > > > > You're right, but this bug was only discovered due to warning on file > > "apt_mark_storage" being missing (i.e. first run of new version of apt), > > and while bug is technically present in current version of apt, it's much > > harder to trigger it. Basically, you have to intentionally break something and > > get a warning. Still, it can probably be moved out to a separate, really tiny > > commit. > > I believe that a separate tiny commit would be better anyway (even if the bug > is difficult to trigger in the original code.) Let's make a separate tiny > commit. > Ok, I'll move it out into a separate commit. > > > commit e61fb72b368b6e6b63b10f84adc2b433b9747a5a > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Fri Dec 8 15:41:32 2017 +0300 > > > > > > Fix apt-mark auto/manual functions in apt-shell > > > > > > Notes: > > > Effects on the old behavior: none > > > > > > commit d536c2da25a6d1cfeb16ec9519d5d5a41a8779e5 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Fri Dec 8 15:45:39 2017 +0300 > > > > > > Use rpm header RPMTAG_AUTOINSTALLED for storing state of 'autoinstalled' > > > flag > > > > > > Notes: > > > Effects on the old behavior: can be either on APT, or on RPM > > > (modifying the database). > > > > > > I haven't thoroughly studied this code. > > > > > > I've noticed an interesting thing for me: that the auto-flags are > > > read/written in a package-by-package manner. Previously, I used to > > > think that there is a way to get the data with one sweep. The writing > > > is simply done in UpdateMarks() by iterating through the packages; and > > > also immediately whenever rpm-install etc is done: see > > > pkgRPMLibPM::AddToTransaction(). The reading happens in > > > rpmListParser::Flags()--for a single package; this function is called > > > in pkgcachegen.cc in pkgCacheGenerator::MergeList() during an > > > iteration over a list of packages like this: > > > > > > NewPackage(Pkg,PackageName) > > > ... > > > Pkg->Flags = List.Flags(); > > > ... > > > > > > whereas NewPackage() allocates a structure for the package and > > > "registers" it in the cache etc. > > > > > > Also, an idea/question appeared to me: if this information is stored > > > in the RPM db, there could be a way to change these flags not by means > > > of APT, but rather through RPM's command-line interface (because these > > > flags make sense without APT, for a substitute of APT; they are even > > > stored in the common RPM db). Is there something like this? vseleznv@ > > > said that there is apparently no way to do this through RPM now, but > > > it has been discussed in RPM mailing lists. In a sense, of course, > > > apt-mark is such a tool, but it has "apt" in its name (and uses APT > > > libs)... > > > > > > > Since this information is now stored as a tag in package header, there's no API > > AFAIK to read this tag for every package at once, > > same goes for adding it. As for modifying the tag's value, you can see ugly > > approach (IIRC, proposed by upstream) at function > > "pkgRPMLibPM::UpdateMarks(...)". You can't just remove tag for a header stored > > at RPMDB, it'll go insane. > > Thanks for this explanation! > > > Yes, currently there are no tools to change these tags via rpm. As mentioned > > above, it doesn't even have a proper API for changing tags. > > It's easy to implement a tool for just one specific tag (and thus one specific > > tag type and tag items count). > > Making a generic tool is a much bigger task, but would it really be needed? > > AFAIK, this is only tag that is getting changed without package reinstallation. > > As for non-generic tool, just for "autoinstalled" tag, the question is > > following: would upstream accept it? I don't think so. And for downstream > > solution apt-mark is not much different from rpm-mark. > > I agree with you. It's not a part of the current task to bother with a special > rpm-only tool for this. I have just noted this, so that vseleznv@ and others > could consider this improvement to rpm. > > > > commit 9f32c9202a062e9549341fb3c044268f601f9ac9 > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > Date: Tue Dec 19 16:28:51 2017 +0300 > > > > > > 0.5.15lorg2-alt59 > > > > > > - Implemented following actions and commands (closes: #34036): > > > "apt-get autoremove", "apt-mark showmanual [package1 ...]", "apt-mark > > > showauto [package1 ...]", > > > "apt-mark manual package1 [package2 ...]", "apt-mark auto package1 > > > [package2 ...]", > > > "apt-mark showstate [package1 ...]". > > > > > > Notes: > > > Some general thoughts on the whole series of changes. > > > > > > 1. It would be interesting to see a comparison of these changes in > > > APT-RPM to the analogous code in Debian APT. It is not wanted to > > > maintain a project which is further diverging. > > > > > > > First commits with moving functions to common libraries are inspired by current > > state of apt-get from Debian, this allows to reduce code duplication, > > To check them, I'm about to repeat your movements myself and then have a look > at the diff between my and your results. (To see whether we get equivalent > code.) No magic tools are unfortunately known to us that would help here. > > In general, it's a problem: how the author of such changes should present them > to prove to the reader that the new code is equivalent to the old code... > > > but autoremove algorithm is self-written, because the one from Debian required > > basically getting whole apt-get from Debian. > > Debian's version is integrated in their package processing, which has changed a > > lot compared to version from ALT. > > It's not a single function at apt-get from Debian, it's basically a part of > > most functions. > > > > I agree that diverging might be unwanted, but basically ALT is upstream of > > apt-rpm, which only looks like an old apt-get. > > > > > 2. For discussion, I'd group the changes into 3 groups: > > > > > > - Refactoring. > > > - Setting the flags during the actions (such as install, dist-upgrade). > > > - Autoremove algorithm and action, apt-mark additional tool. > > > > > > IMO, it's important that the "Setting the flags during the actions > > > (such as install, dist-upgrade)" part works correctly, and--of > > > course--that the refactoring leads to equivalent/better code. As for > > > the autoremove algorithm (the really useful thing for the user), it > > > can be easily modified or re-implemented on top of the flags if the > > > users are not satisfied. (Perhaps, it's even possible experiment in > > > Lua and make use of the auto-flags from Lua, isn't it?) > > > > > > > If Lua API allows to modify Flags of package and thus set/unset flag > > 'autoinstalled' before transaction is committed, > > it should work. But I didn't test it. > > I see. That's not a part of the current task, IMO. > > > > The refactoring changes have not yet been checked thoroughly by me (I > > > need some tools or comments to demonstrate they are OK). > > > > > > After my review, I believe the "Setting the flags during different > > > actions (such as install, dist-upgrade)" does not break the old > > > behavior. > > > > > > As for the interaction with RPM, I don't have a good knowledge of the > > > RPM API to check it. > > > > > > 3. An idea came up to me after reading these changes: it concerns the > > > "Setting the flags during different actions (such as install, > > > dist-upgrade)" part, how to make it more clear and convincing. > > > However, I would not like to suggest it if it diverges from > > > Debian even more. > > > > > > To make sure that the decision how to change the flag was conciously > > > made by the programmer in all cases, I'd add the value of the > > > auto-flag as an obligatory parameter to all functions which create > > > packages or modify their state, such as NewPackage(), MarkInstall() > > > etc. and perhaps everything that modifies StateCache structure. (If > > > the auto-flag value is explicit, then it is an explicit decision how > > > to change the auto-flag on every kind of change and no cases are > > > overlooked.) > > > > Yes, you're correct, additional modifications to apt-get's API can be made to > > include 'autoinstall' value. > > Just need an answer for the question whether API should be changed and diverged > > from Debian further. > > Well, if we agree that such a rewrite would make us (and other people who read > the code) more confident that no case where we need to decide on the value of > the auto-mark has been left unnoticed, then it is worth it (IMO). > > As for the divergence from Debian, it is big already, isn't it? Or is there a > hope that the API is compatible now, so that some code can be transferred? But > if that code does some work with modifying the lists of packages, then it will > need to be adapted to work with our auto-marks. (Otherwise it could leave wrong > marks after its work.) So, it seems that API compatibility is not important in > this case (in things related to modifying the lists of packages and deciding > which mark to set), perhaps even harmful (decisions about the auto-marks could > be left out because the current API doesn't make them obligatory). I have no hope for compatibility with Debian. In my opinion, in order to migrate to the newest apt from Debian, all changes to apt from ALT would have to be redone from scratch for new apt, and tested again, and probably some additional changes would be needed after all testing. (В ответ на комментарий №9) > (In reply to comment #7) > > > > > $ git --no-pager log --reverse gears/sisyphus..autoremove > > > > commit 5d970b58d5ec676ef543f678ab52d1921e3266b0 > > > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > > > Date: Tue Oct 24 13:06:40 2017 +0300 > > > > > > > > Move some common functions out of apt-get.cc to library > > > > > > > > Replace legacy int types. > > > > > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > > > > > Notes: > > > > (TODO: review more thoroughly for new bugs.) Let's assume this is an > > > > equivalent rewrite of the code. > > > > > > > > Effect on the old behavior: must be equivalent (not checked thoroughly). > > > > > > > > 1. Perhaps it would be easier to review if non-similar changes were > > > > placed in separate commits (grouping the similar changes such as: > > > > "Replace legacy int types.") > > > > > > > > 2. Was it necessary to make the changes in spacing (which can be > > > > hidden with git diff -w)? If yes, it would be easier to review if they > > > > were in a separate commit. > > (I believe that we don't want to compare the code to Debian at the first place. > So, then we want it to compare to our old code.) > > So, to make it easier to compare with our old code, could you please split this > commit so that there are separate commits (in any order you like and any number > of individual commits): > > 1. with the relocations and minimal necessary changes (due to the relocations), > so that git show -w --color-moved 5d970b58d5ec676ef543f678ab52d1921e3266b0 > would show all lines as moved (I see them at the new locations in yellow and > cyan; at the old locations -- in magenta); > > 2. with the code improvements (so that "git show" shows no additional noise). > Ok, I'll try splitting code.
(In reply to comment #10) > > Наверное, это не самое важное (сравнение с Debian), но если ты неплохо > > разобрался в этих делах, то ради улучшения понимания участвующих в обсуждении > > не мог бы ты указать на конкретные функции в apt-rpm и в Debian (с тегом, > > коммитом) с небольшим комментарием, что разошлось. (Перечислять все -- это было > > бы долго, но хочется оттолкнуться от какого-нибудь небольшого комментария с > > твоим пониманием, чтобы начать знакомиться с расхождениями. Этот вопрос ещё и > > возник ниже здесь, когда обсуждаем стоит ли API менять ради явного > > обязательного выставления отметок auto при всех затрагивающих список пакетов > > операциях.) > > > > Не до конца понятен вопрос - он про разницу в реализации autoremove или про > разницу в apt вообще? > Самая большая разница в apt, которая сразу приходит на ум, это поддержка > multiarch в новом apt. > Также, как я уже упоминал, apt-get из Debian хранит немало информации в > специальном файле > "Dir::State::extended_states", в то время как apt-get из ALT не хранит в данный > момент > дополнительной информации об установленных пакетах где-либо за пределами rpmdb. > Если же говорить про реализацию autoremove, то в apt-get из Debian логика > autoremove размазана по всему > коду в виде различных оценок (score) и отметок (mark), которые в том числе как > минимум частично > берутся из уже указанного файла "Dir::State::extended_states". > В моей реализации за autoremove отвечают несколько отдельных функций. > К сожалению, на конкретные коммиты тут сложно дать ссылки. Да, вопрос был вызван интересом не к новым нескольким функциям, а к сравнению затронутых уже существовавших функций. Была бы некоторая дополнительная информация к размышлению: мы вот взяли эти функции поменяли ради наших целей так, а в Debian тем временем их поменяли вот так. > > As for the divergence from Debian, it is big already, isn't it? Or is there a > > hope that the API is compatible now, so that some code can be transferred? But > I have no hope for compatibility with Debian. In my opinion, in order to > migrate > to the newest apt from Debian, all changes to apt from ALT would have to be > redone from scratch for new apt, > and tested again, and probably some additional changes would be needed after > all testing. And before this, we would need a good test coverage of our current APT.
Залил новую версию кода на http://git.altlinux.org/people/darktemplar/packages/?p=apt.git;a=summary (В ответ на комментарий №11) > (In reply to comment #10) > > > > Наверное, это не самое важное (сравнение с Debian), но если ты неплохо > > > разобрался в этих делах, то ради улучшения понимания участвующих в обсуждении > > > не мог бы ты указать на конкретные функции в apt-rpm и в Debian (с тегом, > > > коммитом) с небольшим комментарием, что разошлось. (Перечислять все -- это было > > > бы долго, но хочется оттолкнуться от какого-нибудь небольшого комментария с > > > твоим пониманием, чтобы начать знакомиться с расхождениями. Этот вопрос ещё и > > > возник ниже здесь, когда обсуждаем стоит ли API менять ради явного > > > обязательного выставления отметок auto при всех затрагивающих список пакетов > > > операциях.) > > > > > > > Не до конца понятен вопрос - он про разницу в реализации autoremove или про > > разницу в apt вообще? > > Самая большая разница в apt, которая сразу приходит на ум, это поддержка > > multiarch в новом apt. > > Также, как я уже упоминал, apt-get из Debian хранит немало информации в > > специальном файле > > "Dir::State::extended_states", в то время как apt-get из ALT не хранит в данный > > момент > > дополнительной информации об установленных пакетах где-либо за пределами rpmdb. > > Если же говорить про реализацию autoremove, то в apt-get из Debian логика > > autoremove размазана по всему > > коду в виде различных оценок (score) и отметок (mark), которые в том числе как > > минимум частично > > берутся из уже указанного файла "Dir::State::extended_states". > > В моей реализации за autoremove отвечают несколько отдельных функций. > > К сожалению, на конкретные коммиты тут сложно дать ссылки. > > Да, вопрос был вызван интересом не к новым нескольким функциям, а к сравнению > затронутых уже существовавших функций. Была бы некоторая дополнительная > информация к размышлению: мы вот взяли эти функции поменяли ради наших целей > так, а в Debian тем временем их поменяли вот так. > К сожалению, тут сложно просто взять какой-то коммит или функцию и найти где что-то разом сильно изменилось. Изменения обычно небольшие, но их много, очень, и даже небольшие изменения в коде могут дать значительные изменения в работе, а иногда наоборот разные изменения могут вести к одинаковому поведению. Если посмотреть в src-rpm apt-get текущей версии, то там патч к исходникам: 128 files changed, 3374 insertions(+), 814 deletions(-) всего 8640 строк. И это к исходникам, в которых уже есть базовая имплементация apt-rpm, а не к чистому apt-get какой-либо старой версии. Конкретные расхождения apt-get из Debian и из ALT я не искал. > > > As for the divergence from Debian, it is big already, isn't it? Or is there a > > > hope that the API is compatible now, so that some code can be transferred? But > > > I have no hope for compatibility with Debian. In my opinion, in order to > > migrate > > to the newest apt from Debian, all changes to apt from ALT would have to be > > redone from scratch for new apt, > > and tested again, and probably some additional changes would be needed after > > all testing. > > And before this, we would need a good test coverage of our current APT.
(In reply to comment #12) > Залил новую версию кода на > http://git.altlinux.org/people/darktemplar/packages/?p=apt.git;a=summary Спасибо! (Сразу заметил по diff-у с прошлой версией, что часть улучшений кода исчезлаю. Ну впрочем, и фиг с ними, если нет желания их вносить.) Читаю git log -p --color-moved -w gears/sisyphus..darktemplar@ALT/sisyphus --reverse К сожаленью, тут в коммит ba9c1b1e211239630a11768a73b92bc6f4781fef с перемещением кода затесались другие небольшие улучшения, не необходимые для перемещения кода, и из-за этого читателю не ясно. Вижу, что много кода не раскрашено как перемещённый (т.е. yellow, cyan, magenta, blue). Для примера отыскал такое соответствие: Было: - if (Cache[I].Delete() == true) - { - // CNC:2002-07-25 - bool Obsoleted = false; Стало: + if (Cache[I].Delete()) + { + // CNC:2002-07-25 + bool Obsoleted = false; В этом примере первая строчка не раскрашивается как перемещённая, потому что в неё было внесено улучшение (конечно, я согласен, что глупо писать" == true", но вот бы эти улучшения при желании в другой коммит без перемещений). Напомню, хочется, чтобы в git show --color-moved -w ba9c1b1e211239630a11768a73b92bc6f4781fef (Git >= 2.15) все строчки за исключением минимальных необходимых изменений были раскрашены как перемещённые.
Вань, я предлагаю идти по чуть чуть. Давай 1827407ae4c6463031d8c5c46cfa6a32bd02ac7f выложим в Sisyphus - он явно ни не что не влияет и нужен.
(In reply to comment #14) > Вань, я предлагаю идти по чуть чуть. > Давай 1827407ae4c6463031d8c5c46cfa6a32bd02ac7f выложим в Sisyphus - он явно ни > не что не влияет и нужен. Да, здесь всё понятно и замечаний нет. Хотим ли мы сделать лишний релиз пакета без user-visible changes?
(In reply to comment #13) > Вижу, что много кода не раскрашено как перемещённый (т.е. yellow, cyan, > magenta, blue). Для примера отыскал такое соответствие: > Напомню, хочется, чтобы в git show --color-moved -w > ba9c1b1e211239630a11768a73b92bc6f4781fef (Git >= 2.15) все строчки за > исключением минимальных необходимых изменений были раскрашены как перемещённые. Я бы даже про этоот способ проверки написал пояснение в commit message вроде такого: (To check that this commit only moves existing code between files, look at the output of: git show --color-moved -w ...)
(In reply to comment #15) > (In reply to comment #14) > > Вань, я предлагаю идти по чуть чуть. > > Давай 1827407ae4c6463031d8c5c46cfa6a32bd02ac7f выложим в Sisyphus - он явно ни > > не что не влияет и нужен. > > Да, здесь всё понятно и замечаний нет. > > Хотим ли мы сделать лишний релиз пакета без user-visible changes? Тут я вижу такую полезную возможность: сделать коммиты с улучшением кода поверх p8, потом их смёрджить в sisyphus. (Правда, кому-то лишний merge может не очень понравиться.) Но зато не будет преумножения мест, где одно и то же изменение появляется в виде разных коммитов. Собрать релизы и в p8, и в sisyphus, и в c8.1. Да и реализацию marks и autoremove можно поверх p8 сделать, оно же по идее существующее поведение не должно менять, и можно будет потом после проверки в Sisyphus легко собрать релиз для p8.
Бэкпорт функциональность в p8 пока что врятли возможен - там нужно вносить изменения ещё и в rpm, а вот чистка кода - почему бы и нет.
(In reply to comment #17) > Тут я вижу такую полезную возможность: > > сделать коммиты с улучшением кода поверх p8, потом их смёрджить в sisyphus. > (Правда, кому-то лишний merge может не очень понравиться.) Но зато не будет > преумножения мест, где одно и то же изменение появляется в виде разных > коммитов. > Собрать релизы и в p8, и в sisyphus, и в c8.1. > > Да и реализацию marks и autoremove можно поверх p8 сделать, оно же по идее > существующее поведение не должно менять, и можно будет потом после проверки в > Sisyphus легко собрать релиз для p8. Так, например, делал at@ в истории rpm: 4.0.4-alt98.23 <- 4.0.4-alt97.M50.20 <- 4.0.4-alt95.M41.30 <- 4.0.4-alt77.M40.28 http://git.altlinux.org/gears/r/rpm-build.git?p=rpm-build.git;a=commit;h=d5c7cd8f6c6e57a30a1ab227da88b60eb61a9093 Мне такая организация запомнилась и понравилась по идее.
не, Вань, тащить эти коммиты по истории через все бранчи я бы не стал. Они действительно не несут никакой полезной нагрузки без всего остального, да и пусть оно сначала обкатывается в unstable.
(In reply to comment #18) > Бэкпорт функциональность в p8 пока что врятли возможен - там нужно вносить > изменения ещё и в rpm, Согласен, не учёл изменения в rpm. > а вот чистка кода - почему бы и нет. да
(In reply to comment #20) > не, Вань, тащить эти коммиты по истории через все бранчи я бы не стал. > Они действительно не несут никакой полезной нагрузки без всего остального, да и > пусть оно сначала обкатывается в unstable. У меня нет существенных возражений против этого тоже.
Разбил коммит ba9c1b1e211239630a11768a73b92bc6f4781fef на два, заодно нашёл места, где дублированный код не дочистил и поправил и их.
(In reply to comment #23) > Разбил коммит ba9c1b1e211239630a11768a73b92bc6f4781fef на два, заодно нашёл > места, где дублированный код не дочистил и поправил и их. Спасибо! Теперь выглядит понятнее и убедительнее для проверяющих. Заметил из неидентичных строк изменения объявлений функций и изменения про вывод сообщений. Их пока не обдумал; возможно, это надо делать вместе с перемещением. А также немного осталось того, что не стоит менять в этом коммите (потому что это не перемещение кода, а небольшое переписывание-улучшение) -- раскрашено обычным красным (также помимо красных строк указываю тут контекст, т.е. функцию): -void ShowNew(ostream &out,CacheFile &Cache) -{ ... - if (Cache[I].NewInstall() == true) { ... -} -void ShowDel(ostream &out,CacheFile &Cache) -{ ... - if (Cache[I].Delete() == true) ... -} -bool ShowEssential(ostream &out,CacheFile &Cache) -{ ... - if (Cache[I].Delete() == true) ... - if (Cache[P].Delete() == true) ... -} -void Stats(ostream &out,pkgDepCache &Dep) -{ ... - if (Dep[I].NewInstall() == true) - Install++; - else - { - if (Dep[I].Upgrade() == true) - Upgrade++; - else - if (Dep[I].Downgrade() == true) - Downgrade++; - } - // CNC:2002-07-29 - if (Dep[I].Delete() == true) ... - else if ((Dep[I].iFlags & pkgDepCache::ReInstall) == pkgDepCache::ReInstall) ... -} Много красного в: -class CacheFile : public pkgCacheFile -{ ... -} Возможно, закомментированное (красное) не нужно; тогда можно удалить в отдельном предыдущем (или следующем) коммите, а в этом оставить чистые перемещения только: -bool CacheFile::CheckDeps(bool AllowBroken) -{ ... - //if (DCache->DelCount() != 0 || DCache->InstCount() != 0) - // return _error->Error("Internal Error, non-zero counts"); ... (Чем больше красного, тем сложнее отслеживать, что в этом объёмном коммите на самом деле происходит.)
(In reply to comment #24) > (In reply to comment #23) > > Разбил коммит ba9c1b1e211239630a11768a73b92bc6f4781fef на два, заодно нашёл > > места, где дублированный код не дочистил и поправил и их. > > Спасибо! Теперь выглядит понятнее и убедительнее для проверяющих. Это был комментарий к git show --color-moved -w fae1fcbc22c37686c47195cc882a8bb46715c98e > Заметил из неидентичных строк изменения объявлений функций и изменения про > вывод сообщений. Их пока не обдумал; возможно, это надо делать вместе с > перемещением. > > А также немного осталось того, что не стоит менять в этом коммите (потому что > это не перемещение кода, а небольшое переписывание-улучшение) -- раскрашено > обычным красным (также помимо красных строк указываю тут контекст, т.е. > функцию):
(В ответ на комментарий №24) > (In reply to comment #23) > > Разбил коммит ba9c1b1e211239630a11768a73b92bc6f4781fef на два, заодно нашёл > > места, где дублированный код не дочистил и поправил и их. > > Спасибо! Теперь выглядит понятнее и убедительнее для проверяющих. > > Заметил из неидентичных строк изменения объявлений функций и изменения про > вывод сообщений. Их пока не обдумал; возможно, это надо делать вместе с > перемещением. > > А также немного осталось того, что не стоит менять в этом коммите (потому что > это не перемещение кода, а небольшое переписывание-улучшение) -- раскрашено > обычным красным (также помимо красных строк указываю тут контекст, т.е. > функцию): > Тут проблема в том, что эти классы и функции из apt-get.cc и apt-shell.cc не идентичны. В этом можно убедиться просто сравнив их списки аргументов. Например: void ShowUpgraded(ostream &out,CacheFile &Cache) void ShowUpgraded(ostream &out,CacheFile &Cache,pkgDepCache::State *State=NULL) После перемещения получается: void ShowUpgraded(std::ostream &out, std::ostream &c3out, CacheFile &Cache, pkgDepCache::State *State, size_t ScreenWidth) Помимо совмещения параметров, также в параметры добавляются те переменные, которые ранее были глобальными для конкретных файлов (c3out и ScreenWidth). В имплементации, соответственно, различия тоже небольшие имеются. Не получается просто взять версию из одного из файлов. После совмещения этих реализаций и получаются такие "удалённые" строки. Вот разница в оригинальных функциях на примере той же функции ShowUpgraded: -void ShowUpgraded(ostream &out,CacheFile &Cache) +void ShowUpgraded(ostream &out,CacheFile &Cache,pkgDepCache::State *State=NULL) { string List; string VersionsList; for (unsigned J = 0; J < Cache->Head().PackageCount; J++) { pkgCache::PkgIterator I(Cache,Cache.List[J]); - // Not interesting - if (Cache[I].Upgrade() == false || Cache[I].NewInstall() == true) - continue; + if (State == NULL) { + // Not interesting + if (Cache[I].Upgrade() == false || Cache[I].NewInstall() == true) + continue; + } else { + // Not interesting + if (Cache[I].NewInstall() == true || + !(Cache[I].Upgrade() == true && (*State)[I].Upgrade() == false)) + continue; + } List += string(I.Name()) + " "; VersionsList += string(Cache[I].CurVersion) + " => " + Cache[I].CandVersion + "\n"; } - if (!List.empty()) c3out<<"apt-get:upgrade-list:"<<List<<endl; ShowList(out,_("The following packages will be upgraded"),List,VersionsList); } > -void ShowNew(ostream &out,CacheFile &Cache) > -{ > ... > - if (Cache[I].NewInstall() == true) { > ... > -} > > -void ShowDel(ostream &out,CacheFile &Cache) > -{ > ... > - if (Cache[I].Delete() == true) > ... > -} > > -bool ShowEssential(ostream &out,CacheFile &Cache) > -{ > ... > - if (Cache[I].Delete() == true) > ... > - if (Cache[P].Delete() == true) > ... > -} > > -void Stats(ostream &out,pkgDepCache &Dep) > -{ > ... > - if (Dep[I].NewInstall() == true) > - Install++; > - else > - { > - if (Dep[I].Upgrade() == true) > - Upgrade++; > - else > - if (Dep[I].Downgrade() == true) > - Downgrade++; > - } > - // CNC:2002-07-29 > - if (Dep[I].Delete() == true) > ... > - else if ((Dep[I].iFlags & pkgDepCache::ReInstall) == > pkgDepCache::ReInstall) > ... > -} > > Много красного в: > > -class CacheFile : public pkgCacheFile > -{ > ... > -} > > Возможно, закомментированное (красное) не нужно; тогда можно удалить в > отдельном предыдущем (или следующем) коммите, а в этом оставить чистые > перемещения только: > С учётом вышеописанного, для того, чтобы сделать здесь чистые перемещения, необходимо всю работу по предварительной подготовке класса CacheFile и функций ShowList, ShowBroken, ShowNew, ShowDel, ShowKept, ShowUpgraded, ShowDowngraded, ShowHold, ShowEssential, Stats сделать заранее отдельным коммитом, и скорее всего сделать такую работу в двух местах. С учётом небольшого количества таких "неперемещённых" удалений, для меня дублирование этих изменений перед их перемещением выглядит менее желаемым вариантом. > -bool CacheFile::CheckDeps(bool AllowBroken) > -{ > ... > - //if (DCache->DelCount() != 0 || DCache->InstCount() != 0) > - // return _error->Error("Internal Error, non-zero counts"); > ... > > (Чем больше красного, тем сложнее отслеживать, что в этом объёмном коммите на > самом деле происходит.)
Ваня, какие наши дальнейшие действия ?
(In reply to comment #27) > Ваня, какие наши дальнейшие действия ? Я сейчас закончу обновлять замечания (notes), сверяя их с обновлёнными коммитами. В то время, пока я обдумываю объяснения darktemplar@, там будет ещё что причесать. (К тому же, мне поручили помочь с анализом замечаний сертификаторов, и довольно срочно, так что буду как-то совмещать эти дела. Ещё я предложил vseleznv@ последить за этой историей с apt, если будет возможность, я же ему немного помогал разбираться с rpmrebuild. :) )
А ты замечания где-то на вики держишь ?
(In reply to comment #29) Нет, не на вики, а попробовал использовать git notes. (In reply to comment #4) > смотреть изменения в apt. > > Решил попробовать записывать свои заметки в git notes. > > Рассматриваемые коммиты запушил к себе так: > > git push -u @ALT autoremove --follow-tags > > а сохранённые заметки (пока только одну) пришлось так: > > $ git push @ALT refs/notes/commits > remote: gitery-update: Unrecognized ref name: refs/notes/commits > remote: error: hook declined to update refs/notes/commits > To git.alt:packages/apt.git > ! [remote rejected] refs/notes/commits -> refs/notes/commits (hook > declined) > error: failed to push some refs to 'git.alt:packages/apt.git' > $ git push @ALT refs/notes/commits:refs/heads/NOTES > To git.alt:packages/apt.git > * [new branch] refs/notes/commits -> NOTES > $ > > Забрать к себе должно быть можно в обратную сторону: > > git fetch git://git.altlinux.org/people/imz/packages/apt.git > refs/heads/NOTES:refs/notes/commits > > и посмотреть как-то так: > > git log gears/sisyphus..autoremove --reverse > > Тогда вы увидете первую заметку сначала. (При условии, что вы уже имеете > ветку autoremove.) ... > Когда сохранённых заметок будет больше или они будут интересными, пошлю их > по почте или сюда тоже.
(In reply to comment #26) Спасибо за объяснения про перемещения и необходимые изменения. Понятно, что сложно сделать лучше. Обновил заметки, остались замечания (много про отступы), которые хорошо бы учесть: git --no-pager log --reverse gears/sisyphus..autoremove2 commit 1827407ae4c6463031d8c5c46cfa6a32bd02ac7f Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Sep 28 10:43:13 2018 +0300 Replace legacy int types. Based on ideas and code from apt-1.4.8 from Debian. commit fae1fcbc22c37686c47195cc882a8bb46715c98e Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Mon Oct 1 16:44:18 2018 +0300 Move some common functions out of apt-get.cc and apt-shell.cc to library Based on ideas and code from apt-1.4.8 from Debian. Notes: (TODO: review more thoroughly for new bugs.) Let's assume this is an equivalent rewrite of the code. Effect on the old behavior: must be equivalent (not checked thoroughly the modifications on top of the moved code). 1. Perhaps it would be easier to review if non-similar changes were placed in separate commits (grouping the similar changes such as getting rid of the stupid " == true"). This commit should only move some existing code, so that "git show -w --color-moved ...." shows all lines as moved (yellow, cyan, magenta, blue; in Git>=2.15) except for the minimal necessary changes (due to the relocations). A few modifications (in green/red color) still remain in this commit: * function declarations (perhaps, that's necessary when moving the code); * working with the output (is it really necessary or this can be made in a separate commit before or after this one?); * a few " == true" etc. removals still remain in this commit (they should be placed in a separate commit); 2. Please add a note into the commit message concerning the use of "git show -w --color-moved ...." to check this commit; this would be useful for those who read this commit in future. commit 7cff4dac29156640a30e4134d7586be3331f5a84 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Mon Oct 1 18:26:56 2018 +0300 Update common cache functions. Based on ideas and code from apt-1.4.8 from Debian. commit aca5a4901d1c478d257b03abd85724f65a078dd8 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Thu Oct 26 15:58:22 2017 +0300 Add skeleton of apt-mark Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old behavior: This does not change the behavior of old commands, because the calls to read/writeStateFile() do nothing as of now. commit ea5182d2fb0f199938183dd295145595b5496045 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Thu Oct 26 17:48:17 2017 +0300 Apt-mark: implement storing and retrieving list of automatically-installed packages Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old behavior: theoretically possible, because calls to read/writeStateFile() were injected into the old code (in the prev.commit) and they might contain an error/crash. However, practically, if there are no errors (crashes), no direct effect on the old behavior is expected because their return codes do not affect anything at the places where they are called. BTW, writeStateFile() should be a const method, why not? * * * Some additional explanations (but not criticisms; thanks darktemplar@ for the last two explanations): 0. As for the side-effects of writeStateFile(), I was not sure about the effect of things like: return _error->Error(_("Failed to replace file: %s"), AptMarkStorageFileName.c_str()); -- whether this would give a normal return code (which is ignored) or _error->Error() would somehow interrupt the normal execution? No, it won't interrupt the execution -- I've checked the sources for GlobalError::Error. 1. This code uses the filename from the config parameter introduced in the prev.commit like this: Cnf.CndSet("Dir::State::apt_mark_storage", "apt_mark_storage"); Note that it has no dir component in the path. However, when it is retrieved here like this: std::string AptMarkStorageFileName = _config->FindFile("Dir::State::apt_mark_storage"); it is expected to have a non-empty leading directory in the path, and therefore delim_index (the position of '/') is expected to be something "non-trivial". The temporary file is then to be created in the same directory: AptMarkTempFileName = AptMarkStorageFileName.substr(0, delim_index + 1) + ".__tmp__apt_mark_storage"; 2. Why is "iter" not dereferenced here: linestr = iter.Name(); though its other previous uses are written with dereferencing, e.g.: StateCache const &stateCache = PkgState[iter->ID]; This "iter" is an instance of class "pkgCache::PkgIterator", which is not actually a std::iterator. It has function "Name()", which returns package name, and it has "operator->", which returns pointer to another class "Package". It also has other functions. It's defined at cacheiterators.h. commit b41b10efd2502e43ecb289ca68cabd58bbf4e809 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Oct 27 12:11:00 2017 +0300 Don't keep records for packages not installed Allow to toggle marking reinstalled packages as manually reinstalled. Based on ideas and code from apt-1.4.8 from Debian. Notes: quite clear, but: this seems to be 2 logically separate changes Effects on the old behavior: no (only errors in MarkAuto() might theoretically affect it) 1. The spacing (for indentation) is not consistent with how the code is indented nearby in: apt/apt-pkg/contrib/configuration.cc apt/apt/apt-pkg/depcache.cc apt/cmdline/apt-get.cc (the surrounding code is indented with crazy 3 spaces) apt/cmdline/apt-shell.cc (the surrounding code is indented with crazy 3 spaces) (Although I don't like their mix of tabs and spaces, because it assumes that a tab is always 8 spaces... but we must not break the surrounding spacing/indentation rules.) I'd also have a look at the current indentation practice in the current Debian APT sources, and if they are more sensible I would probably re-indent according to their rules all our sources before introducing new changes. (In a hope to exchange some minor patches.) commit 3e1f1ef8946dbcc001110906bacf9fafe5632b7c Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Oct 27 16:34:17 2017 +0300 apt-get: implement handling of flag 'auto' for upgrade and dist-upgrade actions Fix apt-get install and dependency fixer to not mark packages manually installed when they're actually auto-installed. Based on ideas and code from apt-1.4.8 from Debian. Notes: 1. The parameter name "AptMarkInheritance" does not clarify whether the "auto" or "manual" mark is set on the condition named by the value of the parameter (like "at_least_one" etc.); I mean that something like "AptAutoMarkInheritance" would make it more clear for users looking at the configuration. Has this paramater name been copied from Debian? If so, then it's better not change it, probably, although it might be not very clear. TODO: review the rest of the code more thoroughly. 2. The spacing (with tabs) is not consistent with how the code is indented nearby in: apt/apt-pkg/algorithms.cc apt/apt-pkg/depcache.cc commit b05d7e2bedc62c06c76394dabf4790958886deb9 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Oct 27 18:05:47 2017 +0300 apt-get: implement 'autoremove' action Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old behavior: no (it's a new command) (As for the algorithm, the interested users will be testing it when using.) commit 14d38a7c35b3e5ce7cf63dd3b19ab0d4a19bac09 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Tue Oct 31 16:41:23 2017 +0300 Add option to provide debug information for "apt-get autoremove" virtual dependencies solving Based on ideas and code from apt-1.4.8 from Debian. Notes: quite clear Effects on the old behavior: no commit 1a05fac5aa97b01df3bffb7cf4190defd0c163ee Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Tue Oct 31 17:35:25 2017 +0300 Apt-shell: implement autoremove action Based on ideas and code from apt-1.4.8 from Debian. Notes: quite clear 1. The indentation spacing doesn't match the surrounding code in: apt/cmdline/apt-shell.cc (they use 3 spaces) commit fb5c7f3493eef49a519a307a6fbf88e3703b93b3 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Thu Nov 2 11:05:44 2017 +0300 apt-get autoremove: improve dependencies calculation, ensure that versions and version ops are properly processed When testing autoremove feature, I've found out that one of python packages was replaced by another one, and on next autoremove run, it was replaced back. It were packages "python-strict" and "python-relaxed", they both provide "python = 2.7", so it was ordered to uninstall since package named "python" is present in system. But there is a call to "Fix.Resolve(...)" at the end of autoremove function which plays role of additional sanity check that system dependencies are all satisfied, and this function proposed to install another package, i.e. if "python-strict" is removed by autoremove, "python-relaxed" is installed, and vice versa. And autoremove shouldn't install any packages, it clearly was a bug. But package "python" has different version, it provides "python = 2.7.14-alt7", and thus checking for specific provided version of virtual provide "python" (and any other virtual provide, if it has dependency on specific version; solution is generic) fixed this case. Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old behavior: no (This is just a change in the calculation algorithm for the new autoremove feature.) 1. The indentation spacing doesn't match the old code in: apt/apt-pkg/algorithms.cc (they use 3 spaces) commit 18c9ad342cf8fee5e0a76f5c6ba1468cce809ee6 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Nov 3 10:44:28 2017 +0300 apt-shell: implement functions of apt-mark Based on ideas and code from apt-1.4.8 from Debian. Notes: Effects on the old bahavior: none 1. Let's make more lines shown as moved by git show -w --color-moved: use the same variable name (say, "DepCache", and not "Cache" or "Dep") in one of the old commits as in this one after the movement. 2. Write this hint down in the commit message (to view with "git show -w --color-moved") so that future readers can reproduce our experience. commit 2b4eb6fe1c7429d21ceb3f825ffde1ae850ad7d3 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Sep 28 15:54:20 2018 +0300 apt-shell: fix bug causing apt-shell to fail to start in case of warnings being produced. Based on ideas and code from apt-1.4.8 from Debian. Notes: ok, clear commit e1f11375615a11fce7d45b7f64d0aeb01d6bcd79 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Dec 8 15:41:32 2017 +0300 Fix apt-mark auto/manual functions in apt-shell Notes: Effects on the old behavior: none commit ce7103b27a35915123f984c4b85c3de47afb08ab (HEAD) Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Dec 8 15:45:39 2017 +0300 Use rpm header RPMTAG_AUTOINSTALLED for storing state of 'autoinstalled' flag Notes: Effects on the old behavior: can be either on APT, or on RPM (modifying the database). I haven't thoroughly studied this code. 1. The indentations spacing doesn't match the surrounding file in: apt/apt-pkg/luaiface.cc apt/apt-pkg/packagemanager.h apt/apt-pkg/rpm/rpmlistparser.cc apt/apt-pkg/rpm/rpmpm.cc apt/cmdline/apt-shell.cc Some observations (not criticisms) follow: * * * I've noticed an interesting thing for me: that the auto-flags are read/written in a package-by-package manner. Previously, I used to think that there is a way to get the data with one sweep. The writing is simply done in UpdateMarks() by iterating through the packages; and also immediately whenever rpm-install etc is done: see pkgRPMLibPM::AddToTransaction(). The reading happens in rpmListParser::Flags()--for a single package; this function is called in pkgcachegen.cc in pkgCacheGenerator::MergeList() during an iteration over a list of packages like this: NewPackage(Pkg,PackageName) ... Pkg->Flags = List.Flags(); ... whereas NewPackage() allocates a structure for the package and "registers" it in the cache etc. darktemplar@: Since this information is now stored as a tag in package header, there's no API AFAIK to read this tag for every package at once, same goes for adding it. As for modifying the tag's value, you can see ugly approach (IIRC, proposed by upstream) at function "pkgRPMLibPM::UpdateMarks(...)". You can't just remove tag for a header stored at RPMDB, it'll go insane. *** Also, an idea/question appeared to me: if this information is stored in the RPM db, there could be a way to change these flags not by means of APT, but rather through RPM's command-line interface (because these flags make sense without APT, for a substitute of APT; they are even stored in the common RPM db). Is there something like this? vseleznv@ said that there is apparently no way to do this through RPM now, but it has been discussed in RPM mailing lists. In a sense, of course, apt-mark is such a tool, but it has "apt" in its name (and uses APT libs)... darktemplar@ (and I agree): Yes, currently there are no tools to change these tags via rpm. As mentioned above, it doesn't even have a proper API for changing tags. It's easy to implement a tool for just one specific tag (and thus one specific tag type and tag items count). Making a generic tool is a much bigger task, but would it really be needed? AFAIK, this is only tag that is getting changed without package reinstallation. As for non-generic tool, just for "autoinstalled" tag, the question is following: would upstream accept it? I don't think so. And for downstream solution apt-mark is not much different from rpm-mark. commit 62bc03097ec743f0ffe223a88d28ae4f802bbc86 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Tue Dec 19 16:28:51 2017 +0300 0.5.15lorg2-alt59 - Implemented following actions and commands (closes: #34036): "apt-get autoremove", "apt-mark showmanual [package1 ...]", "apt-mark showauto [package1 ...]", "apt-mark manual package1 [package2 ...]", "apt-mark auto package1 [package2 ...]", "apt-mark showstate [package1 ...]". Notes: Some general thoughts on the whole series of changes. 1. We are not trying to track the individual changes which bring further divergence with Debian because it's too difficult given the already existing huge divergence... (That's how I understand darktemplar@'s answers.) So, we shouldn't take this into account when making decisions about our changes. 2. For discussion, I'd group the changes into 3 groups: - Refactoring. - Setting the flags during the actions (such as install, dist-upgrade). - Autoremove algorithm and action, apt-mark additional tool. IMO, it's important that the "Setting the flags during the actions (such as install, dist-upgrade)" part works correctly, and--of course--that the refactoring leads to equivalent/better code. As for the autoremove algorithm (the really useful thing for the user), it can be easily modified or re-implemented on top of the flags if the users are not satisfied. (Perhaps, it's even possible experiment in Lua and make use of the auto-flags from Lua, isn't it?) The refactoring changes have not yet been checked thoroughly by me (although there is substantial progress thanks to the discovery of "git diff -w --color-moved" in Git>=2.15 and thanks to darktemplar@ splitting the commit into just moving the code and--separately--improving the code). After my review, I believe the "Setting the flags during different actions (such as install, dist-upgrade)" does not break the old behavior. As for the interaction with RPM, I don't have a good knowledge of the RPM API to check it. 3. An idea came up to me after reading these changes: it concerns the "Setting the flags during different actions (such as install, dist-upgrade)" part, how to make it more clear and convincing. (I'll try to suggest such a change in the code. Let's not worry if it diverges from Debian even more.) To make sure that the decision how to change the flag was consciously made by the programmer in all cases, I'd add the value of the auto-flag as an obligatory parameter to all functions which create packages or modify their state, such as NewPackage(), MarkInstall() etc. and perhaps everything that modifies StateCache structure. (If the auto-flag value is explicit, then it is an explicit decision how to change the auto-flag on every kind of change and no cases are overlooked.) commit cf79677b2245737430b72963f56b93a44990d613 Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Fri Dec 22 17:33:57 2017 +0300 Fix crash on fail to read package file commit cab2f7ada144dd357c21a5da1470a026245a09ec (darktemplar@ALT/sisyphus, @ALT/autoremove2, autoremove2) Author: Aleksei Nikiforov <darktemplar@altlinux.org> Date: Thu Jan 11 12:01:10 2018 +0300 0.5.15lorg2-alt60 - Fixed crash on fail to read package file.
(В ответ на комментарий №31) > (In reply to comment #26) > > Спасибо за объяснения про перемещения и необходимые изменения. Понятно, что > сложно сделать лучше. > > Обновил заметки, остались замечания (много про отступы), которые хорошо бы > учесть: > > git --no-pager log --reverse gears/sisyphus..autoremove2 > commit 1827407ae4c6463031d8c5c46cfa6a32bd02ac7f > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Sep 28 10:43:13 2018 +0300 > > Replace legacy int types. > > Based on ideas and code from apt-1.4.8 from Debian. > > commit fae1fcbc22c37686c47195cc882a8bb46715c98e > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Mon Oct 1 16:44:18 2018 +0300 > > Move some common functions out of apt-get.cc and apt-shell.cc to library > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > (TODO: review more thoroughly for new bugs.) Let's assume this is an > equivalent rewrite of the code. > > Effect on the old behavior: must be equivalent (not checked thoroughly > the modifications on top of the moved code). > > 1. Perhaps it would be easier to review if non-similar changes were > placed in separate commits (grouping the similar changes such as > getting rid of the stupid " == true"). This commit should only > move some existing code, so that "git show -w --color-moved ...." > shows all lines as moved (yellow, cyan, magenta, blue; in Git>=2.15) > except for the minimal necessary changes (due to the relocations). > > A few modifications (in green/red color) still remain in this commit: > > * function declarations (perhaps, that's necessary when moving the > code); > > * working with the output (is it really necessary or this can be made > in a separate commit before or after this one?); > > * a few " == true" etc. removals still remain in this commit (they > should be placed in a separate commit); > Please see comment #26, specifically diff for two implementations of function ShowUpgraded(...). It is where those 'removals' are coming from. Unless those functions are accordingly modified in a separate previous commit, these changes would remain. I don't think duplicating these changes worth the effort. > 2. Please add a note into the commit message concerning the use of > "git show -w --color-moved ...." to check this commit; this would be > useful for those who read this commit in future. > Done. > commit 7cff4dac29156640a30e4134d7586be3331f5a84 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Mon Oct 1 18:26:56 2018 +0300 > > Update common cache functions. > > Based on ideas and code from apt-1.4.8 from Debian. > > commit aca5a4901d1c478d257b03abd85724f65a078dd8 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Thu Oct 26 15:58:22 2017 +0300 > > Add skeleton of apt-mark > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old behavior: This does not change the behavior of old > commands, because the calls to read/writeStateFile() do nothing as of > now. > > commit ea5182d2fb0f199938183dd295145595b5496045 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Thu Oct 26 17:48:17 2017 +0300 > > Apt-mark: implement storing and retrieving list of automatically-installed > packages > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old behavior: theoretically possible, because calls to > read/writeStateFile() were injected into the old code (in the > prev.commit) and they might contain an error/crash. However, > practically, if there are no errors (crashes), no direct effect on the old > behavior is expected because their return codes do not affect anything > at the places where they are called. > > BTW, writeStateFile() should be a const method, why not? > > * * * > > Some additional explanations (but not criticisms; thanks darktemplar@ > for the last two explanations): > > 0. As for the side-effects of writeStateFile(), I was not sure about the > effect of things like: > > return _error->Error(_("Failed to replace file: %s"), > AptMarkStorageFileName.c_str()); > > -- whether this would give a normal return code (which is ignored) or > _error->Error() would somehow interrupt the normal execution? No, it > won't interrupt the execution -- I've checked the sources for > GlobalError::Error. > > 1. This code uses the filename from the config parameter > introduced in the prev.commit like this: > > Cnf.CndSet("Dir::State::apt_mark_storage", "apt_mark_storage"); > > Note that it has no dir component in the path. However, when it is > retrieved here like this: > > std::string AptMarkStorageFileName = > _config->FindFile("Dir::State::apt_mark_storage"); > > it is expected to have a non-empty leading directory in the path, > and therefore delim_index (the position of '/') is expected to be > something "non-trivial". The temporary file is then to be created in > the same directory: > > AptMarkTempFileName = AptMarkStorageFileName.substr(0, delim_index + 1) + > ".__tmp__apt_mark_storage"; > > 2. Why is "iter" not dereferenced here: > > linestr = iter.Name(); > > though its other previous uses are written with dereferencing, e.g.: > > StateCache const &stateCache = PkgState[iter->ID]; > > This "iter" is an instance of class "pkgCache::PkgIterator", which is not > actually a std::iterator. > It has function "Name()", which returns package name, and it has > "operator->", > which returns pointer to another class "Package". > It also has other functions. It's defined at cacheiterators.h. > > commit b41b10efd2502e43ecb289ca68cabd58bbf4e809 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Oct 27 12:11:00 2017 +0300 > > Don't keep records for packages not installed > > Allow to toggle marking reinstalled packages as manually reinstalled. > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > quite clear, but: this seems to be 2 logically separate changes > Split it into two separate commits. > Effects on the old behavior: no > (only errors in MarkAuto() might theoretically affect it) > > 1. The spacing (for indentation) is not consistent with how the code > is indented nearby in: > > apt/apt-pkg/contrib/configuration.cc > apt/apt/apt-pkg/depcache.cc It's completely consistent in scope of single function. If it's still unacceptable, I'll reformat it. > apt/cmdline/apt-get.cc (the surrounding code is indented with crazy 3 > spaces) > apt/cmdline/apt-shell.cc (the surrounding code is indented with crazy 3 > spaces) > Could you please describe why this code is considered inconsistently formatted? This whole function is already badly formatted, but IMO added lines are formatted similarly to nearby lines of code. > (Although I don't like their mix of tabs and spaces, because it > assumes that a tab is always 8 spaces... but we must not break the > surrounding spacing/indentation rules.) > > I'd also have a look at the current indentation practice in the > current Debian APT sources, and if they are more sensible I would > probably re-indent according to their rules all our sources before > introducing new changes. (In a hope to exchange some minor patches.) > I've chosen randomly file private-cmndline.cc from Debian's sources of APT, and it still uses 3-spaces identation and sometimes tabs. Still horrible IMO. I don't like idea of reformatting all code. Still, if it has to be done, it's better to do it using automated tools, like astyle or clang-format or something else. > commit 3e1f1ef8946dbcc001110906bacf9fafe5632b7c > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Oct 27 16:34:17 2017 +0300 > > apt-get: implement handling of flag 'auto' for upgrade and dist-upgrade > actions > > Fix apt-get install and dependency fixer to not mark packages manually > installed when they're actually auto-installed. > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > 1. The parameter name "AptMarkInheritance" does not clarify whether the > "auto" or "manual" mark is set on the condition named by the value of > the parameter (like "at_least_one" etc.); I mean that something like > "AptAutoMarkInheritance" would make it more clear for users looking at > the configuration. Has this paramater name been copied from Debian? If > so, then it's better not change it, probably, although it might be not > very clear. > > TODO: review the rest of the code more thoroughly. > > 2. The spacing (with tabs) is not consistent with how the code is > indented nearby in: > > apt/apt-pkg/algorithms.cc > apt/apt-pkg/depcache.cc > Fixed formatting in function pkgDistUpgrade. In other functions formatting is consistent for the whole function. Same as above, this can be changed. > commit b05d7e2bedc62c06c76394dabf4790958886deb9 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Oct 27 18:05:47 2017 +0300 > > apt-get: implement 'autoremove' action > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old behavior: no (it's a new command) > > (As for the algorithm, the interested users will be testing it when using.) > > commit 14d38a7c35b3e5ce7cf63dd3b19ab0d4a19bac09 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Tue Oct 31 16:41:23 2017 +0300 > > Add option to provide debug information for "apt-get autoremove" virtual > dependencies solving > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > quite clear > > Effects on the old behavior: no > > commit 1a05fac5aa97b01df3bffb7cf4190defd0c163ee > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Tue Oct 31 17:35:25 2017 +0300 > > Apt-shell: implement autoremove action > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > quite clear > > 1. The indentation spacing doesn't match the surrounding code in: > > apt/cmdline/apt-shell.cc (they use 3 spaces) > Same as above. > commit fb5c7f3493eef49a519a307a6fbf88e3703b93b3 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Thu Nov 2 11:05:44 2017 +0300 > > apt-get autoremove: improve dependencies calculation, ensure that versions > and version ops are properly processed > > When testing autoremove feature, I've found out that one of python packages > was > replaced by another one, and on next autoremove run, it was replaced back. > It were packages "python-strict" and "python-relaxed", they both provide > "python = 2.7", so it was ordered to uninstall since package named "python" > is > present in system. > But there is a call to "Fix.Resolve(...)" at the end of autoremove function > which plays role of additional sanity check that system dependencies are > all > satisfied, > and this function proposed to install another package, i.e. if > "python-strict" > is removed by autoremove, "python-relaxed" is installed, and vice versa. > And > autoremove shouldn't install any packages, it clearly was a bug. > But package "python" has different version, it provides "python = > 2.7.14-alt7", > and thus checking for specific provided version of virtual provide "python" > (and any other virtual provide, if it has dependency on specific version; > solution is generic) fixed this case. > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old behavior: no > (This is just a change in the calculation algorithm for the new > autoremove feature.) > > 1. The indentation spacing doesn't match the old code in: > > apt/apt-pkg/algorithms.cc (they use 3 spaces) > Same as above. > commit 18c9ad342cf8fee5e0a76f5c6ba1468cce809ee6 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Nov 3 10:44:28 2017 +0300 > > apt-shell: implement functions of apt-mark > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > Effects on the old bahavior: none > > 1. Let's make more lines shown as moved by git show -w --color-moved: > use the same variable name (say, "DepCache", and not "Cache" or "Dep") in > one of the old commits as in this one after the movement. > Is it really necessary to make additional changes and commits here? It's not stated that it's just a move of code from file to file, and some changes are made due to change of type of variable: in removed code it was a pointer to class, now it's a reference. > 2. Write this hint down in the commit message > (to view with "git show -w --color-moved") so that future readers > can reproduce our experience. > > commit 2b4eb6fe1c7429d21ceb3f825ffde1ae850ad7d3 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Sep 28 15:54:20 2018 +0300 > > apt-shell: fix bug causing apt-shell to fail to start in case of warnings > being produced. > > Based on ideas and code from apt-1.4.8 from Debian. > > Notes: > ok, clear > > commit e1f11375615a11fce7d45b7f64d0aeb01d6bcd79 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Dec 8 15:41:32 2017 +0300 > > Fix apt-mark auto/manual functions in apt-shell > > Notes: > Effects on the old behavior: none > > commit ce7103b27a35915123f984c4b85c3de47afb08ab (HEAD) > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Dec 8 15:45:39 2017 +0300 > > Use rpm header RPMTAG_AUTOINSTALLED for storing state of 'autoinstalled' > flag > > Notes: > Effects on the old behavior: can be either on APT, or on RPM > (modifying the database). > > I haven't thoroughly studied this code. > > 1. The indentations spacing doesn't match the surrounding file in: > > apt/apt-pkg/luaiface.cc > apt/apt-pkg/packagemanager.h > apt/apt-pkg/rpm/rpmlistparser.cc > apt/apt-pkg/rpm/rpmpm.cc > apt/cmdline/apt-shell.cc > Same as above: if it's a separate entity (function, class, etc), I prefer to adopt a better formatting, even if it doesn't match the rest of project. If it's really necessary, this can be changed. > Some observations (not criticisms) follow: > > * * * > > I've noticed an interesting thing for me: that the auto-flags are > read/written in a package-by-package manner. Previously, I used to > think that there is a way to get the data with one sweep. The writing > is simply done in UpdateMarks() by iterating through the packages; and > also immediately whenever rpm-install etc is done: see > pkgRPMLibPM::AddToTransaction(). The reading happens in > rpmListParser::Flags()--for a single package; this function is called > in pkgcachegen.cc in pkgCacheGenerator::MergeList() during an > iteration over a list of packages like this: > > NewPackage(Pkg,PackageName) > ... > Pkg->Flags = List.Flags(); > ... > > whereas NewPackage() allocates a structure for the package and > "registers" it in the cache etc. > > darktemplar@: > > Since this information is now stored as a tag in package header, there's no > API > AFAIK to read this tag for every package at once, > same goes for adding it. As for modifying the tag's value, you can see ugly > approach (IIRC, proposed by upstream) at function > "pkgRPMLibPM::UpdateMarks(...)". You can't just remove tag for a header > stored > at RPMDB, it'll go insane. > > *** > > Also, an idea/question appeared to me: if this information is stored > in the RPM db, there could be a way to change these flags not by means > of APT, but rather through RPM's command-line interface (because these > flags make sense without APT, for a substitute of APT; they are even > stored in the common RPM db). Is there something like this? vseleznv@ > said that there is apparently no way to do this through RPM now, but > it has been discussed in RPM mailing lists. In a sense, of course, > apt-mark is such a tool, but it has "apt" in its name (and uses APT > libs)... > > darktemplar@ (and I agree): > > Yes, currently there are no tools to change these tags via rpm. As > mentioned > above, it doesn't even have a proper API for changing tags. > It's easy to implement a tool for just one specific tag (and thus one > specific > tag type and tag items count). > Making a generic tool is a much bigger task, but would it really be needed? > AFAIK, this is only tag that is getting changed without package > reinstallation. > As for non-generic tool, just for "autoinstalled" tag, the question is > following: would upstream accept it? I don't think so. And for downstream > solution apt-mark is not much different from rpm-mark. > > commit 62bc03097ec743f0ffe223a88d28ae4f802bbc86 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Tue Dec 19 16:28:51 2017 +0300 > > 0.5.15lorg2-alt59 > > - Implemented following actions and commands (closes: #34036): > "apt-get autoremove", "apt-mark showmanual [package1 ...]", "apt-mark > showauto [package1 ...]", > "apt-mark manual package1 [package2 ...]", "apt-mark auto package1 > [package2 ...]", > "apt-mark showstate [package1 ...]". > > Notes: > Some general thoughts on the whole series of changes. > > 1. We are not trying to track the individual changes which bring further > divergence with Debian because it's too difficult given the already > existing huge divergence... (That's how I understand darktemplar@'s > answers.) So, we shouldn't take this into account when making > decisions about our changes. > > 2. For discussion, I'd group the changes into 3 groups: > > - Refactoring. > - Setting the flags during the actions (such as install, dist-upgrade). > - Autoremove algorithm and action, apt-mark additional tool. > > IMO, it's important that the "Setting the flags during the actions > (such as install, dist-upgrade)" part works correctly, and--of > course--that the refactoring leads to equivalent/better code. As for > the autoremove algorithm (the really useful thing for the user), it > can be easily modified or re-implemented on top of the flags if the > users are not satisfied. (Perhaps, it's even possible experiment in > Lua and make use of the auto-flags from Lua, isn't it?) > > The refactoring changes have not yet been checked thoroughly by me > (although there is substantial progress thanks to the discovery of > "git diff -w --color-moved" in Git>=2.15 and thanks to darktemplar@ > splitting the commit into just moving the code > and--separately--improving the code). > > After my review, I believe the "Setting the flags during different > actions (such as install, dist-upgrade)" does not break the old > behavior. > > As for the interaction with RPM, I don't have a good knowledge of the > RPM API to check it. > > 3. An idea came up to me after reading these changes: it concerns the > "Setting the flags during different actions (such as install, > dist-upgrade)" part, how to make it more clear and convincing. > > (I'll try to suggest such a change in the code. Let's not worry if it > diverges from Debian even more.) > > To make sure that the decision how to change the flag was consciously > made by the programmer in all cases, I'd add the value of the > auto-flag as an obligatory parameter to all functions which create > packages or modify their state, such as NewPackage(), MarkInstall() > etc. and perhaps everything that modifies StateCache structure. (If > the auto-flag value is explicit, then it is an explicit decision how > to change the auto-flag on every kind of change and no cases are > overlooked.) I don't think NewPackage() needs to be changed. As for MarkInstall*() functions family, it may be changed, but there is a lot of places where those functions are used to install package updates, for example, and it'd basically look like "get current AUTO flag state via getMarkAuto() function and pass it to MarkInstall*() function in order to keep flag intact". If that's fine, I can try doing this. > > commit cf79677b2245737430b72963f56b93a44990d613 > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Fri Dec 22 17:33:57 2017 +0300 > > Fix crash on fail to read package file > > commit cab2f7ada144dd357c21a5da1470a026245a09ec (darktemplar@ALT/sisyphus, > @ALT/autoremove2, autoremove2) > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > Date: Thu Jan 11 12:01:10 2018 +0300 > > 0.5.15lorg2-alt60 > > - Fixed crash on fail to read package file.
(In reply to comment #32) > (В ответ на комментарий №31) > > (In reply to comment #26) > > > > Спасибо за объяснения про перемещения и необходимые изменения. Понятно, что > > сложно сделать лучше. > > > > Обновил заметки, остались замечания (много про отступы), которые хорошо бы > > учесть: > > * a few " == true" etc. removals still remain in this commit (they > > should be placed in a separate commit); > > > > Please see comment #26, specifically diff for two implementations of function > ShowUpgraded(...). > It is where those 'removals' are coming from. Unless those functions are > accordingly modified > in a separate previous commit, these changes would remain. > I don't think duplicating these changes worth the effort. I see. Sorry, I haven't updated this note after reading your reply. > > > 2. Please add a note into the commit message concerning the use of > > "git show -w --color-moved ...." to check this commit; this would be > > useful for those who read this commit in future. > > > > Done. Thanks! > > 1. The spacing (for indentation) is not consistent with how the code > > is indented nearby in: > > > > apt/apt-pkg/contrib/configuration.cc > > apt/apt/apt-pkg/depcache.cc > > It's completely consistent in scope of single function. If it's still > unacceptable, I'll reformat it. It would be better if the formatting was the same in the whole file. (Of course, there might be some mess already from other contributors in the past, but despite that, let's format your new code according to the dominant style in each file. If you add a completely new file, then I think it's acceptable with whatever number of spaces or tabs you like.)
(В ответ на комментарий №33) > (In reply to comment #32) > > (В ответ на комментарий №31) > > > (In reply to comment #26) > > > > > > Спасибо за объяснения про перемещения и необходимые изменения. Понятно, что > > > сложно сделать лучше. > > > > > > Обновил заметки, остались замечания (много про отступы), которые хорошо бы > > > учесть: > > > > * a few " == true" etc. removals still remain in this commit (they > > > should be placed in a separate commit); > > > > > > > Please see comment #26, specifically diff for two implementations of function > > ShowUpgraded(...). > > It is where those 'removals' are coming from. Unless those functions are > > accordingly modified > > in a separate previous commit, these changes would remain. > > I don't think duplicating these changes worth the effort. > > I see. Sorry, I haven't updated this note after reading your reply. > > > > > > 2. Please add a note into the commit message concerning the use of > > > "git show -w --color-moved ...." to check this commit; this would be > > > useful for those who read this commit in future. > > > > > > > Done. > > Thanks! > > > > > 1. The spacing (for indentation) is not consistent with how the code > > > is indented nearby in: > > > > > > apt/apt-pkg/contrib/configuration.cc > > > apt/apt/apt-pkg/depcache.cc > > > > It's completely consistent in scope of single function. If it's still > > unacceptable, I'll reformat it. > > It would be better if the formatting was the same in the whole file. (Of > course, there might be some mess already from other contributors in the past, > but despite that, let's format your new code according to the dominant style in > each file. If you add a completely new file, then I think it's acceptable with > whatever number of spaces or tabs you like.) I've fixed formatting everywhere I could find. If I still missed something, please let me know.
*** Bug 14005 has been marked as a duplicate of this bug. ***
I've updated commit "Allow to toggle marking reinstalled packages as manually reinstalled.": Marking is moved to common place which reduces code duplication and allows it to correctly work even if SetReInstall(...) function is called from scripts. I've also updated luaiface.cc in commit "apt-get: implement handling of flag 'auto' for upgrade and dist-upgrade actions". (В ответ на комментарий №31) > 3. An idea came up to me after reading these changes: it concerns the > "Setting the flags during different actions (such as install, > dist-upgrade)" part, how to make it more clear and convincing. > > (I'll try to suggest such a change in the code. Let's not worry if it > diverges from Debian even more.) > > To make sure that the decision how to change the flag was consciously > made by the programmer in all cases, I'd add the value of the > auto-flag as an obligatory parameter to all functions which create > packages or modify their state, such as NewPackage(), MarkInstall() > etc. and perhaps everything that modifies StateCache structure. (If > the auto-flag value is explicit, then it is an explicit decision how > to change the auto-flag on every kind of change and no cases are > overlooked.) > I've implemented such change as an experiment. As external API change it sounds good, but for APT itself, for over 20 calls to MarkInstall, only in 4 places I had to set a possibly different value, at all other occurences I had to make a construct to preserve current value. In order to remove call to getMarkAuto(...) in such cases, I've implemented special value 'DontChange'. It's available at "auto_flag" branch.
(In reply to comment #36) > I've updated commit "Allow to toggle marking reinstalled packages as manually > reinstalled.": > Marking is moved to common place which reduces code duplication and allows it > to correctly work > even if SetReInstall(...) function is called from scripts. > > I've also updated luaiface.cc in commit "apt-get: implement handling of flag > 'auto' for upgrade and dist-upgrade actions". > > (В ответ на комментарий №31) > > 3. An idea came up to me after reading these changes: it concerns the > > "Setting the flags during different actions (such as install, > > dist-upgrade)" part, how to make it more clear and convincing. > > > > (I'll try to suggest such a change in the code. Let's not worry if it > > diverges from Debian even more.) > > > > To make sure that the decision how to change the flag was consciously > > made by the programmer in all cases, I'd add the value of the > > auto-flag as an obligatory parameter to all functions which create > > packages or modify their state, such as NewPackage(), MarkInstall() > > etc. and perhaps everything that modifies StateCache structure. (If > > the auto-flag value is explicit, then it is an explicit decision how > > to change the auto-flag on every kind of change and no cases are > > overlooked.) > > > > I've implemented such change as an experiment. As external API change it sounds > good, > but for APT itself, for over 20 calls to MarkInstall, only in 4 places I had to > set a possibly different value, > at all other occurences I had to make a construct to preserve current value. > In order to remove call to getMarkAuto(...) in such cases, I've implemented > special value 'DontChange'. > It's available at "auto_flag" branch. Thank you! I'm going to look into this later today. Apart from being good for an external API, it might also help a programmer who contributes to the APT sources and can't hold all the details in his head. This way the API requires him to make a decision. That's what I was thinking about when I suggested this.
(In reply to comment #32) > > commit b41b10efd2502e43ecb289ca68cabd58bbf4e809 > > Author: Aleksei Nikiforov <darktemplar@altlinux.org> > > Date: Fri Oct 27 12:11:00 2017 +0300 > > > > Don't keep records for packages not installed > > > > Allow to toggle marking reinstalled packages as manually reinstalled. > > > > Based on ideas and code from apt-1.4.8 from Debian. > > > > Notes: > > quite clear, but: this seems to be 2 logically separate changes > > > > Split it into two separate commits. > > > Effects on the old behavior: no > > (only errors in MarkAuto() might theoretically affect it) > > > > 1. The spacing (for indentation) is not consistent with how the code > > is indented nearby in: > > > > apt/apt-pkg/contrib/configuration.cc > > apt/apt/apt-pkg/depcache.cc > > It's completely consistent in scope of single function. If it's still > unacceptable, I'll reformat it. > > > apt/cmdline/apt-get.cc (the surrounding code is indented with crazy 3 > > spaces) > > apt/cmdline/apt-shell.cc (the surrounding code is indented with crazy 3 > > spaces) > > > > Could you please describe why this code is considered inconsistently formatted? > This whole function is already badly formatted, but IMO added lines are > formatted similarly to nearby lines of code. There were 4 spaces (and not 3) :) as indentation in the line with: MarkAuto(Pkg, false); Now this code disappeared from these places in 5def6e5a227058aa65cbdf18dae7055c5ffc85fa , and appeared in a different place with correct indentation in 2fa835085101167cea0db38f6c027913af4dfdbf .
Вань, если всё хорошо и проблема исправлена - то чего мы ждём ? :)
(В ответ на комментарий №39) > Вань, если всё хорошо и проблема исправлена - то чего мы ждём ? :) +1 Может, пора передать в отдел тестирования?
Ждём, чтобы я внимательно изучил новые изменения. (Offtopic: Пока что я просто был занят последние дни другим: подготовкой к запуску инструмента для отслеживания замечаний, который мы с Денисом решили просто как можно скорее научиться запускать в том виде, который авторы дают. Потом будет приспособление для нас -- это другое дело и в промежутке можно сделать другие дела. Затягивать оба дела я не вижу смысла, переключаясь между ними. Одно запущу, а это не такое большое дело, просто осталось несколько ошибок при компиляции; после этого сделаю очередной подход к APT autoremove. Я ожидаю, что те ошибки уже сегодня преодолею.)
I've pushed my recent technical Git notes about individual commits, and these issues must have been resolved already in the recent darktemplar@'s branch. On a higher level: Your history of work is not so interesting. The presentation of patches is important. I'm putting them in a more sensible order, commenting on them and suggesting sqashes. # Bugfixes Make them first (to be cherry-pickable) as separate commits: 18fa00eb1 Fix crash on fail to read package file 1e01f9ecc apt-shell: fix bug causing apt-shell to fail to start in case of warnings being produced. # Refactoring Keep them as separate commits (each of them makes sense) 1827407ae Replace legacy int types. b05017a52 Rework code to common state b14acf81f Move some common functions out of apt-get.cc and apt-shell.cc to library 2235ada21 Update common cache functions. # apt-mark (closely related to storage) This is a simple UI which could allow testing the storage. (Apart from being useful users.) It introduces apt-mark command as well (will it make sense to split apt-mark from the skeleton?): 79daed4e9 Add skeleton of apt-mark Squash with the first introduction of apt-mark (79daed4e9) e35b0e124 apt-shell: implement functions of apt-mark e229d16d0 Fix apt-mark auto/manual functions in apt-shell # Storage of auto-marks It removes a parameter that has just been introduced (in the prev.commit) -- no need for this: 0ff190e46 Apt-mark: implement storing and retrieving list of automatically-installed packages Can be kept as a separate commit (at least, it makes sense) or squashed with the previous one: 7a96dffa0 Don't keep records for packages not installed We want to keep the previous way (storing in a file) as a working variant in a commit, but then override it with the new way; that's understandable: b32034fde Use rpm header RPMTAG_AUTOINSTALLED for storing state of 'autoinstalled' flag # Marking (when managing packages): Don't touch the braces (that's not related to the essence of this change): 673d17c35 Allow to toggle marking reinstalled packages as manually reinstalled. This also concerns "install" action; please, edit the summary: 1c8ba73fc apt-get: implement handling of flag 'auto' for upgrade and dist-upgrade actions It's good because it breaks the API (say, for synaptic). But the place with SetReInstall()/MarkAuto() is not ideal because it calls MarkAuto() instead of supplying the desired value as an argument to MarkInstall(), but we don't know how to rewrite it better given the current APT functions: d5c130958 Add required Auto flag to MarkInstall function. # autoremove algorithm and commands 5aa1437bf apt-get: implement 'autoremove' action 5b648409b Add option to provide debug information for "apt-get autoremove" virtual dependencies solving Luckily, all common functions have been put in the right place already, so this commit is small and there is no movement here; that's fine: 7fb2e0798 Apt-shell: implement autoremove action 3446c2fe8 apt-get autoremove: improve dependencies calculation, ensure that versions and version ops are properly processed # Release No need to publish 2 releases in the end: 949e1e8f7 0.5.15lorg2-alt59 523943606 0.5.15lorg2-alt60
For futire: It would be interesting to have a mode of running "apt-get install" when no auto-marks are changed. (A configuration option.)
I believe that the new configuration options should be documented in the same commits in the documentation.
It would be nice to transform the new "showauto", "showmanual", "showstate" command into special flags of the available "ls"/"list" command. Then, instead of new extra UI commands we would have an extension of an old UI command (already familiar to some people, so probably simpler to learn). And the new "ls" options could be combined with the old "ls" option to give more specific output. It works like this (with globs, too): $ apt-shell Reading Package Lists... Done Building Dependency Tree... Done Welcome to the APT shell. Type "help" for more information. apt> ls -h Usage: list/ls [options] [pattern ...] List packages matching the given patterns, or all packages if no pattern is given. Wildcards are accepted. Options: -h This help text. -i Show only installed packages. -u Show only installed packages that are upgradable. -v Show installed and candidate versions. -s Show summaries. -g Show packages with group name. -G=? Show packages in specified group. -o=? Set an arbitary configuration option, eg -o dir::cache=/tmp apt> ls -g apt System/Configuration/Packaging apt apt> ls -i apt apt apt> ls -i apt* apt apt-conf-sisyphus apt-printchanges apt-repo apt-repo-tools apt> ls apt* apt apt-cacher-ng apt-conf-etersoft-common apt-conf-mithraen apt-indicator apt-repo apt-scripts aptitude-doc apt-autoclean apt-conf-autoimports-sisyphus apt-conf-etersoft-hold apt-conf-sisyphus apt-log apt-repo-tools apt-scripts-nvidia apt-blacklist apt-conf-branch apt-conf-ignore-systemd apt-conf-tmp-cache apt-printchanges apt-rsync aptitude apt> One possible way to implement this: "showstate" is analoguous to "ls -g": -g Show packages with group name. "showauto"/"showmanual" are analoguos to "ls -G=?" -G=? Show packages in specified group. The flags could be: -a -A=auto -A=manual
I've rearranged commits as proposed, except for d5c130958 Add required Auto flag to MarkInstall function. which stayed at the end of history as was discussed, as separate change indicating API change. Documentation is also updated, it's available in english man page for apt.conf. It's good interface enhancement ideas, but I'd prefer to work on these improvements after main functionality is finished.
(В ответ на комментарий №46) > > It's good interface enhancement ideas, but I'd prefer to work on these > improvements after main functionality is finished. +1
За чем сейчас дело стоит ?
Только что закончил и подготовил со своей стороны всё, что хотелось: Сделал разбиение коммита по приведению к общему состоянию. Так, мне кажется, понятнее читать и отслеживать, что ничего не испорчено. Ветка autoremove12 По-моему, с моей стороны больше нет пожеланий и замечаний к тому, как представлена работа в apt и что там происходит. Можно отправлять.
(In reply to comment #37) > (In reply to comment #36) > > I've updated commit "Allow to toggle marking reinstalled packages as manually > > reinstalled.": > > Marking is moved to common place which reduces code duplication and allows it > > to correctly work > > even if SetReInstall(...) function is called from scripts. > > > > I've also updated luaiface.cc in commit "apt-get: implement handling of flag > > 'auto' for upgrade and dist-upgrade actions". > > > > (В ответ на комментарий №31) > > > 3. An idea came up to me after reading these changes: it concerns the > > > "Setting the flags during different actions (such as install, > > > dist-upgrade)" part, how to make it more clear and convincing. > > > > > > (I'll try to suggest such a change in the code. Let's not worry if it > > > diverges from Debian even more.) > > > > > > To make sure that the decision how to change the flag was consciously > > > made by the programmer in all cases, I'd add the value of the > > > auto-flag as an obligatory parameter to all functions which create > > > packages or modify their state, such as NewPackage(), MarkInstall() > > > etc. and perhaps everything that modifies StateCache structure. (If > > > the auto-flag value is explicit, then it is an explicit decision how > > > to change the auto-flag on every kind of change and no cases are > > > overlooked.) > > > > > > > I've implemented such change as an experiment. As external API change it sounds > > good, > > but for APT itself, for over 20 calls to MarkInstall, only in 4 places I had to > > set a possibly different value, > > at all other occurences I had to make a construct to preserve current value. > > In order to remove call to getMarkAuto(...) in such cases, I've implemented > > special value 'DontChange'. > > It's available at "auto_flag" branch. > > Thank you! I'm going to look into this later today. > > Apart from being good for an external API, it might also help a programmer who > contributes to the APT sources and can't hold all the details in his head. This > way the API requires him to make a decision. That's what I was thinking about > when I suggested this. BTW, Debian also has such a parameter in MarkInstall now (called FromUser): bool MarkDelete(PkgIterator const &Pkg, bool MarkPurge = false, unsigned long Depth = 0, bool FromUser = true); bool MarkInstall(PkgIterator const &Pkg,bool AutoInst = true, unsigned long Depth = 0, bool FromUser = true, bool ForceImportantDeps = false); It is also present in MarkDelete(). I have looked into the code to see what its effect on MarkDelete() is, and found a piece of code that might have an interesting idea: if (FromUser == false) { VerIterator const PV = P.InstVerIter(*this); if (PV.end() == false) { // removed metapackages mark their dependencies as manual to prevent in "desktop depends browser, texteditor" // the removal of browser to suggest the removal of desktop and texteditor.
Выглядит нормально. Пересобрал со всеми последними изменениями задание #199955 и отдал на тестирование.
Обновился, попробовал, работает. apt-mark тоже работает (особенно удобно для всяких poppler'ов)
Коллеги, всем спасибо! Ждём в сизифе после тестирования.
apt-0.5.15lorg2-alt59 -> sisyphus: Tue Dec 19 2017 Aleksei Nikiforov <darktemplar@altlinux> 0.5.15lorg2-alt59 - Fixed crash on fail to read package file. - Implemented following actions and commands (closes: #34036): "apt-get autoremove", "apt-mark showmanual [package1 ...]", "apt-mark showauto [package1 ...]", "apt-mark manual package1 [package2 ...]", "apt-mark auto package1 [package2 ...]", "apt-mark showstate [package1 ...]".
Ура! Алексей, большое спасибо. Хорошо бы анонсировать в списках, на канале, на форуме.
