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

Allow adding repositories recursively #40

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Hummer12007
Copy link

@Hummer12007 Hummer12007 commented Apr 4, 2017

@dol-sen
Copy link
Contributor

dol-sen commented Apr 4, 2017

overall, looks good. just the one configparser issue

try:
import configparser
except ImportError:
import ConfigParser as configparser
Copy link
Contributor

Choose a reason for hiding this comment

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

You added configparser import, but I don't see it used in the remaining patch.

Doe sit get used in a later patch? I haven't looked yet. If so, it should be in the patch that uses it.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, just noticed it (a leftover).

@Hummer12007 Hummer12007 force-pushed the recursive branch 2 times, most recently from 1ca81f8 to 39b73c6 Compare April 4, 2017 21:04
@dol-sen
Copy link
Contributor

dol-sen commented Apr 4, 2017

How were you thinking of dealing with the main repo? in this case "gentoo"

We have been trying to get way from the main-repo configuration.
I'll poke Zac to have a look at this, see what he thinks is the best way.
I'm thinking in terms of portability to other distros, there are a bunch of them, ie: funtoo, sabayon, calculate,...

@zmedico
Copy link
Member

zmedico commented Apr 4, 2017

lgtm

@Hummer12007
Copy link
Author

Hummer12007 commented Apr 4, 2017

I was thinking (about other distros) and figured out that there's no universal solution (apart from looking at repos.conf). And anyway, it just prompts before adding each repo, so the user will have to react (and willingfully accept the repo).
Or we could just special case gentoo (as almost every overlay has it in masters).

@dol-sen
Copy link
Contributor

dol-sen commented Apr 4, 2017

oh, instead of hard coding the special case.

Why don't we add another config item for it. Then it is easily changed...

provided-masters:

it can be a list like the old overlays and some other variables in the config
That should take care of Funtoo's new split repo stuff they are working on.

@Hummer12007 Hummer12007 force-pushed the recursive branch 2 times, most recently from 7d17fce to b848857 Compare April 4, 2017 23:27
@Hummer12007
Copy link
Author

Done.

@zmedico
Copy link
Member

zmedico commented Apr 5, 2017

apart from looking at repos.conf

If it's acceptable to use the portage api, then something like this would work:

Python 3.4.5 (default, Mar 12 2017, 00:44:50) 
[GCC 4.9.3] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import portage
>>> 'gentoo' in portage.settings.repositories
True

@Hummer12007
Copy link
Author

From what I've seen, portage APIs are only used in a module or two (not in "core" layman).

masters = '='.join(s[1:]).strip().split(' ')
break
return list(set(masters) -
set(self.config['provided_masters'].strip().split(' ')))
Copy link
Member

Choose a reason for hiding this comment

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

I would perform filtering like this inside the LaymanAPI.add_repos method, since otherwise this get_masters method is not usable as a means to get a full set of masters.

Copy link
Author

Choose a reason for hiding this comment

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

Right

if (len(s) < 2): continue
k = s[0].strip()
if (k == 'masters'):
masters = '='.join(s[1:]).strip().split(' ')
Copy link
Member

Choose a reason for hiding this comment

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

Generally, it's better to use split() instead of split(' '), since split() works correctly with multiple spaces and tabs.

@dol-sen
Copy link
Contributor

dol-sen commented Apr 5, 2017

Yeah, I tried to keep from using portage api's as much as possible. That way it would be easier to incorporate and.or use in different package managers. The news system is configurable to the point that a custom module could be used, but defaults to the portage API.

This looks really good now.
Thank you Zac, my mail client (claws-mail) no longer self fetches unless I manually change directories, then back. So I missed notification of the changes.

I like it. Zac, by using the provided_masters config, it makes it possible for users to add masters not under layman control, not just the main "gentoo" repo.

@zmedico
Copy link
Member

zmedico commented Apr 5, 2017

I like it. Zac, by using the provided_masters config, it makes it possible for users to
add masters not under layman control, not just the main "gentoo" repo.

It you have a way to query the names of configured repositories then you don't need that. For example, the app-portage/gentoopm package provides a gentoopmq tool that can do it:

$ gentoopmq repositories
gentoo local python

@Hummer12007
Copy link
Author

Hummer12007 commented Apr 9, 2017

Problems may arise in some corner cases when using gentoopmq API, seems to me, esp. when the user has several package managers installed, additionally, in case the downstream distro maintains their own version of the portage tree (named differently than 'gentoo', albeit kept in ~sync with the upstream tree).
IIRC, funtoo does (or did) exactly that.

@dol-sen
Copy link
Contributor

dol-sen commented Apr 9, 2017

Funtoo is in the process of splitting the repository up into logical groups. That they feel would be better for maintaining things like a stable xorg. A user could then also exclude certain sub-repos they don't need. The split repos could then use branches for different stability stages instead of just {'',-arch,~arch, arch}
We could eventually set up a means to specify the means to obtaining the installed list via a pkg manager similar to what I did for the news.

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.

None yet

3 participants