void-linux / xbps

The X Binary Package System (XBPS)
https://voidlinux.org/xbps/
Other
816 stars 124 forks source link

Memory-sync mode still opens some files for writing #474

Open 0x5c opened 2 years ago

0x5c commented 2 years ago

When using xbps-install with flags -MS, xbps will open the db (/var/db/xbps/) in write mode if a remote repo has changed since the last real sync.

This is apparent when running the command without root, where the openat() call fails. It also seems that when run as root, the same command does not actually use the opened file, as further runs of it without root will still present the error. Using strace, the error (which I think comes from lib/download.c) seems to be this:

openat(AT_FDCWD, "x86_64-repodata.part", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0644) = -1 EACCES (Permission denied)

It is in the retry branch of that if though, so maybe there's also an error before it.

There's also a secondary bug; this failure (both sync modes without root) causes a message that both claims failure and success

ERROR: [reposync] failed to fetch file `https://privatereporedacted/x86_64-repodata': Success
[DEBUG] [rpool] `https://privatereporedacted' failed to fetch repository data: Success

(the error line is part of normal output)

I have saved the full strace output of memorysync with and without root, and rootless normal sync. I can provide those if needed but not publicly over github.

Chocimier commented 2 years ago

Arguably this is a feature, manpage can be read as:

Regardless of whether above interpretation is correct, for in-memory sync -M without -S is sufficent, and, unlike combining the two, is present in corpus: https://github.com/void-linux/xbps/issues?q=is:issue+Mun, https://github.com/void-linux/void-packages/issues?q=is:issue+Mun

0x5c commented 2 years ago

-M means "don't read cache, fetch remotes"

This seems like a weird interpretation, since the 1st sentence about -M in the manpage of xbps-src is For remote repositories, the data is fetched and stored in memory for the current operation.. "Stored in memory" definitely means there's no writing to disk.

for

-S means "fetch remotes, write cache"

and

for in-memory sync -M without -S is sufficent,

This does not appear to be the case; -M alone shows the help page since no action is specified (makes sense since -M appears to be a modifier on syncing operations), and -Mund (in-memory, update, dry-run, debug) seems to indicate no fetch happens at all, for any repository. This may be wrong and it actually fetches remotes, but the output is clearly different from a normal sync, and would then have a couple UX implications:

  1. If -M is enough to have the sync happen
    1. It and -S should be mutually exclusive.
    2. It should be an operation in itself (just like -S but without the writes, or an error message if no operation), even if it would be useless when used alone.
  2. A flag that present as not being an operation turns out to be an hidden and silent operation when paired with a second operation like installing or updating. Not dramatic since you need the flag in the command line, but it also means there's no feedback whatsoever. Even debug output isn't clear here, and I can't tell from it whether or not a fetch was done.

As it currently seem to behave; where -Su is "sync, then update packages" which results in a remote fetch, a db update, and a package update, -Mu is "modify Sync to be in-memory, then update packages" which should not result in remote fetch since the sync operation wasn't there to be modified.

tl;dr There's either a bug, or inconsistent UX and flag handling

Chocimier commented 2 years ago

-M means "don't read cache, fetch remotes"

This seems like a weird interpretation, since the 1st sentence about |-M| in the manpage of |xbps-src| is |For remote repositories, the data is fetched and stored in memory for the current operation.|.

You are right. I meant "(don't read cache), (fetch remotes)" rather than "don't (read cache, fetch remotes)".

This does not appear to be the case; |-M| alone shows the help page since no action is specified (makes sense since |-M| appears to be a modifier on syncing operations), and |-Mund| (in-memory, update, dry-run, debug) seems to indicate no fetch happens at all, for any repository. This may be wrong and it actually fetches remotes, but the output is clearly different from a normal sync, and would then have a couple UX implications:

Here -Mund takes significantly longer than -und,lists more packages to update and prints lines like below, so it is enough to fetch.

 [DEBUG] [rpool] checking `https://host/current' at index 0
 [DEBUG] [repo] `https://host/current' used remotely (kept in memory).
  1. If |-M| is enough to have the sync happen
    1. It and |-S| should be mutually exclusive.

Possibly.

 2. It should be an operation in itself (just like |-S| but without the writes, or an error message if no operation), even if it would be useless when used alone.

Possibly not, avoiding costly noop is a good thing.

tl;dr There's either a bug, or inconsistent UX and flag handling

Yes, either not writing cache with -MS or allowing to combine them.

0x5c commented 2 years ago

Here -Mund takes significantly longer than -und,lists more packages to update and prints lines like below, so it is enough to fetch.

There was no difference here when testing today, but a fresh-ish sync and a fast connection to the mirrors could be why.

Possibly not, avoiding costly noop is a good thing.

What would be the costly noop here? If the printing the error would be it, the current behaviour is already to print the help when only -M is given (but without any indication as to why)