Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Improve pkgdb_access API #2313

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improve pkgdb_access API #2313

wants to merge 3 commits into from

Conversation

vstakhov
Copy link
Member

pkgdb works with multirepo, however, it's API is not aware that we sometimes want to work with some specific repo and not with all of them

The idea of this PR is to allow all pkgdb_* functions to be able to select a set of repos to work with (e.g. that could be overriden from CLI args).

The only option for now is to disable repos in the configuration which might be not convenient for some use cases.

@vstakhov vstakhov marked this pull request as ready for review September 17, 2024 12:11
`pkgdb` works with multirepo, however, it's API is not aware that we
sometimes want to work with some specific repo and not with all of them

The idea of this PR is to allow all `pkgdb_*` functions to be able
to select a set of repos to work with (e.g. that could be overriden
from CLI args).

The only option for now is to disable repos in the configuration which
might be not convenient for some use cases.
@vstakhov vstakhov changed the base branch from vstakhov-add-clang-format to main September 17, 2024 13:22
src/upgrade.c Show resolved Hide resolved
@bapt
Copy link
Member

bapt commented Sep 17, 2024

I am puzzled about the usage of va_args here, I both like and dislike it

@vstakhov
Copy link
Member Author

I am puzzled about the usage of va_args here, I both like and dislike it

I have the same feeling. The alternative is to use a list variable or even const char **. varargs are convenient if it comes to converting of the code, as PKGDB_LOCAL access will remain the same. On the other hand it can lead to potential issues as we hide all type checks. I have no strong opinion on that - it's quite easy to do it in either way. In general, we should allow multiple -r arguments everywhere. This functionality is now merely allowed by playing with enabled flags for repos. However, I think it might be neat to allow better CLI handling of multi-repo.

@bapt
Copy link
Member

bapt commented Sep 17, 2024

if we want to allow multiple -r argument (which I like) I think it would be easier to have a const char ** than a va_arg. I fail at seeing an easy way to convert multiple -r into a simple call to pkgdb_access with the current API, but I have only given it a few seconds of thinking.

@olevole
Copy link

olevole commented Sep 17, 2024

FYI: The current patch fixes my problem when I specify a custom/alternative repository (+ -U -n combination) :

pkg update -f -r Test-latest (ok)
pkg upgrade -r Test-latest (ok)
pkg upgrade -r Test-latest -n (ok)
pkg upgrade -r Test-latest -U (ok)
pkg upgrade -r Test-latest -U -n (not OK, error):
pkg: Repository FreeBSD missing. 'pkg update' required

Without these changes, pkg also handles the other repository (which may be broken)

@vstakhov
Copy link
Member Author

I think it would be easier to have a const char ** than a va_arg

However, if we want to reuse the existing const char *reponame that exists in all utilities it will make all calls very ugly. Maybe we should just pass ucl_object_t * to this API representing the set of interesting repos? It will slightly complicate the callers code but not more than char ** transition. However, we will have a set with useful properties. E.g. we can override priorities or do something useful with it without modifying the API. WDYT?

@bapt
Copy link
Member

bapt commented Sep 19, 2024

we have pkg_object as an opaque structure to hide ucl_object_t, we could use it to hide libucl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants