manjaro / pacman-mirrors

This repo has been archived. Our code is now hosted at
https://gitlab.manjaro.org
GNU General Public License v3.0
25 stars 16 forks source link

check no-update function #102

Closed philmmanjaro closed 7 years ago

philmmanjaro commented 7 years ago

We have some issues with the --no-update function. In post_upgrade we use pacman-mirrors -g --no-update. This however results somehow in a new generated mirrorlist and might lead to an outdated mirror. When we update pacman-mirrors we shouldn't change the given mirrorlist. This will prevent some issues we currently have.

fhdk commented 7 years ago

The --no-update on itself does nothing

## When set to True prevents the regeneration of the mirrorlist if
## pacman-mirrors is invoked with the --no-update argument.
## Useful if you don't want the mirrorlist regenerated after a
## pacman-mirrors package upgrade.
# NoUpdate = False

The --no-update requires NoUpdate = True in pacman-mirrors.conf

If you use the recently added --no-mirrorlist argument you have the desired effect

fhdk commented 7 years ago

If you use the --no-mirrorlist argument you have the desired effect

philmmanjaro commented 7 years ago

Are you sure?

phil@manjaro ~/dev/git/manjaro/basic/pacman-mirrors $ sudo pacman-mirrors --no-mirrorlist
.: Info Downloading mirrors from repo.manjaro.org
.: Info Schreibe Mirrorliste
   Germany         : https://mirror.netzspielplatz.de/manjaro/packages/unstable
.: Info Mirrordatei generiert und gesichert in: /etc/pacman.d/mirrorlist
fhdk commented 7 years ago

No - will check immediately - I think it is connected to the api - try adding -a

philmmanjaro commented 7 years ago

The issue is: When we update our system and pacman-mirrors regenerates the mirrorlist file it may lead to an outdated mirror and then the update might fail. Therefore it is better not to edit the mirrorlist on post_upgrade. Downloading a new list of available mirrors however might make sense. The generation of the mirrorlist however should be triggered by the user himself when already installed.

philmmanjaro commented 7 years ago

There is no function --no-mirrorlist. Also the -no-update should work with the switch only and is needed for package pacman-mirrors update thru pacman. Using a switch in our config makes no sense at all. So we have to fix that and make the function work without the need to configure it first.

fhdk commented 7 years ago

The users pacman databases reflects the former primary mirror. Thus when changing the mirrorlist we remove the symbiotic relationship between the users computer and the primary mirror leading to partial updates and a broken user system.

I see where you are going.

I you will do it at you have done it up till now you can do

pacman-mirrors --geoip --no-update -u

Now I remember why I wanted to call it -y --sync instead of update because it makes no sense at all to present to args --update and --no-update which refers to two completely different concepts

philmmanjaro commented 7 years ago

Whatever we call it. It just needs to make sense. We might want to have the latest mirrors-yaml file downloaded, however /etc/pacman.d/mirrorlist should only be touched by first install of the package or when the user wants it. This created a lot of unneeded issues for our users.

Since I use Netzspielplatz mirror, I don't care, as it is always the same mirror and syncs every 5 mins. Others might not.

fhdk commented 7 years ago

There is no function --no-mirrorlist.

Yes there is :) it is tied to api so -a -n will skip mirrorlist but since you have scripts every where relying on --no-update it must be changed so it works as you intend.

Also the -no-update should work with the switch only and is needed for package pacman-mirrors update thru pacman. Using a switch in our config makes no sense at all. So we have to fix that and make the function work without the need to configure it first.

I can't say what has been the idea but that can be changed. However there are users which rely on config - Handys allservers script for example - if you look at the old pacman-mirrors.conf you will see that --no-update has always been connected with the conf.

This created a lot of unneeded issues for our users.

Agree - I will change the 4.2.0 version to

Breaking changes to previously introduced behavior

  1. disregard the --no-update setting in config and break execution if present as argument
  2. change the -u --update to reflect what it does -y --sync

With these backwards breaking changes is the version then 5.0.0?

philmmanjaro commented 7 years ago

Ok, then we keep the logic. However I need some option to update the package pacman-mirrors within a regular System update without generation of mirrorlist. If --no-mirrorlist is the switch to go, then it is fine with me. Simply put: with a post_upgrade we should not touch /etc/pacman.d/mirrorlist.

fhdk commented 7 years ago

Do you want to keep --no-update as is (the allservers script)?

And change the regular updates to use -a --no-mirrorlist?

Should I remove --no-updates dependency on the config entry NoUpdate = True?

philmmanjaro commented 7 years ago

--no-mirrorlist seems not documented. If the switch available is --no-update then we should remove the need of NoUpdate = True in our conf. However the naming was in the past as no update of mirrorlist. Since we now also update the available mirrors in our yaml file the naming now might confuse. So using some like -no-mirrorlist makes sense. Handy may update his script as we are upstream and decide if a switch makes sense or not. The naming we give our functions must make some sense and don't confuse people.

philmmanjaro commented 7 years ago

Simply put: --no-mirrorlist might be the function I need to have the mirrorlist not updated. We may then think about dropping --no-update completely in v4.2.0 as not needed anymore. This will give Handy time to adopt, as removing a function leads to a version bump of our application.

fhdk commented 7 years ago

In docs/index.md in the API section

-n, --no-mirrorlist
Skip mirrorlist and exit when tasks has been done.

Just remember --no-mirrorlist will not work without --api argument :)

philmmanjaro commented 7 years ago

Ok, so it is no bug and I simply have to use pacman-mirrors --api -n in my PKGBUILD for post_upgrade function?

fhdk commented 7 years ago

We can get the best from both worlds by

  1. remove the --no-update option from conf and arguments
  2. remove --no-mirrorlist dependency on --api
  3. pacman-mirrors --no-mirrorlist will download the new json-files and exit

So your system update

pacman-mirrors --no-mirrorlist

which will effectively update the users mirrorfiles (mirrors.json and status.json) and then exit

fhdk commented 7 years ago

@philmmanjaro I have changed behavior in master branch 4.2.0-dev

I have pushed to 4.1.x-stable

This is not a breaking change for 4.1.0-stable - a minor behind the scenes improvement :)

philmmanjaro commented 7 years ago

For v4.1.x it is fine as it is. I simply use --api --no-mirrorlist in post_upgrade function, or does this not download the current status.json file? For v4.2.x I'll use --no-mirrorlist.

fhdk commented 7 years ago

It does not download any files - it works as you expected --no-update to - exit - no change

philmmanjaro commented 7 years ago

Ok, sounds fine. However the source-code checks are partly broken in master and 4.1.x-stable branch. Maybe you adopt them to your made changes. I assume the given code now works :+1:

fhdk commented 7 years ago

Let me check and report back

fhdk commented 7 years ago

@philmmanjaro Check - all green

philmmanjaro commented 7 years ago

Ok, for master it is, not for stable ;)

fhdk commented 7 years ago

I am sorry I can't see what is failing - make test exits clean

Is it a command a screwed up?

@philmmanjaro Found it and fixed

philmmanjaro commented 7 years ago

Hmm, seems still failing ... https://github.com/manjaro/pacman-mirrors/issues/100

fhdk commented 7 years ago

Annoying as they are - they are harmless.

Wait 5 and I have fixed the tests.

And they fixes is up

fhdk commented 7 years ago

The dev package does need the same change as the stable package.

pacman-mirrors --no-mirrorlist
udeved commented 7 years ago

https://github.com/manjaro/packages-core/commit/4b34b260896fcb5e9e838f24dd738319c7f7a47a

I added alpm hooks for the dev package currently, but they work for default pacman-mirros too if required.