Skip to content

Conversation

waebbl
Copy link
Contributor

@waebbl waebbl commented Oct 12, 2018

Based off of gentooPackage.py from =app-portage/tatt-0.5 package,
see https://github.com/gentoo/tatt/blob/master/tatt/gentooPackage.py

Add tests

I played around a bit with how they implemented this in the tatt package. Feel free to use it, if you like it. I also added some tests for the new methods.
Feedback is welcome, if you like anything to be improved.

First tried their current implementation using portage magic, when I noticed this won't build the package on travis, due to missing portage. So I went back to the implementation that's used in ther current tatt-0.5 package.

Note, this implementation doesn't currently need / use the AtomException. As you can see from the tests, it should now support almost all atoms which are to be accepted by portage, a category is no longer needed, thus =package-1.0 is a valid atom, like in portage.
On the other hand, due to just omitting a leading = sign, a user might also give atoms, which would be incorrect in portage, like category/package-1.0. This could be a source to add exception handling, but, as the = get's removed anyway, I didn't see this necessary.

@waebbl waebbl requested a review from nicolasbock as a code owner October 12, 2018 18:10
@nicolasbock
Copy link
Owner

First off, that's awesome!! Thanks! I am going to take some time to carefully read through this, but at first glance it looks really nice.

Reading through your description I noticed that you mention that now something like package-version is also a valid atom. Currently we are unmasking an atom by directly writing to portage.accept_keywords. As far as I know (and I have to double check that when I have more time) this technique requires a category. On the other hand, if we were to use flaggie to do the writing then any valid atom is good. What do you think?

@waebbl
Copy link
Contributor Author

waebbl commented Oct 12, 2018

Take your time to check the code.

You're probably right, you are having a good point here! I didn't think on the implication that we're writing to /etc/portage/package.* files. My comment was to mention the way a user can specify an atom on the command line to pass to ebuildtester.
I'm not using flaggie in day to day work, just took a quick look at it, and it looks like it could handle this. It only requires a valid portage atom, which =<pkg-name>-<version> actually is. The way it's done now, writing the /etc/portage/package.* files before any packages are installed would no longer be possible with this, although I must say, I haven't done a functional test so far. I will test this and see whether I can come up with the needed changes for docker.py.

@nicolasbock
Copy link
Owner

That would be awesome! Regarding the install. We won't have to touch accept_keywords in order to install flaggie. And right now already packages are installed in two phases, one for the base packages and one for the package(s) the user requested. Maybe this will just work (TM) 😉

@waebbl
Copy link
Contributor Author

waebbl commented Oct 14, 2018

I looked a little more into this. The point is, flaggie can't enable test FEATURE, it can only handle USE flag, KEYWORD and LICENSE settings. So it looks like we actually need the user to use a category within his atom.
Another way would be to globally enable the test FEATURE, which, in my opinion wouldn't be a good idea either.

@waebbl
Copy link
Contributor Author

waebbl commented Oct 27, 2018

I updated the PR a little. Looks like we indeed need the user to specify a category to be on the safe side. So I changed the implementation to make the category non-optional.

@waebbl waebbl force-pushed the atom-handling branch 3 times, most recently from 23a74f5 to bbbedad Compare October 27, 2018 21:30
Based off of gentooPackage.py from =app-portage/tatt-0.5 package,
see https://github.com/gentoo/tatt/blob/master/tatt/gentooPackage.py

Add tests
linxon pushed a commit to linxon/ebuildtester that referenced this pull request Feb 23, 2020
Now it requires gentoopm from app-portage/gentoopm package,
see https://github.com/mgorny/gentoopm/

Add tests

Closes: nicolasbock#99
linxon pushed a commit to linxon/ebuildtester that referenced this pull request Feb 23, 2020
Now it requires gentoopm from app-portage/gentoopm package,
see https://github.com/mgorny/gentoopm/

Add tests

Closes: nicolasbock#99
Closes: nicolasbock#96
@linxon linxon mentioned this pull request Feb 23, 2020
linxon pushed a commit to linxon/ebuildtester that referenced this pull request Feb 23, 2020
Now it requires gentoopm from app-portage/gentoopm package,
see https://github.com/mgorny/gentoopm/

Add tests

Closes: nicolasbock#99
Closes: nicolasbock#96
linxon pushed a commit to linxon/ebuildtester that referenced this pull request Feb 23, 2020
Now it requires gentoopm from app-portage/gentoopm package,
see https://github.com/mgorny/gentoopm/

Add tests

Closes: nicolasbock#99
Closes: nicolasbock#96
linxon pushed a commit to linxon/ebuildtester that referenced this pull request Feb 23, 2020
linxon pushed a commit to linxon/ebuildtester that referenced this pull request Feb 23, 2020
linxon pushed a commit to linxon/ebuildtester that referenced this pull request Mar 6, 2020
linxon pushed a commit to linxon/ebuildtester that referenced this pull request Mar 11, 2020
@github-actions
Copy link

This PR is stale because it has been open for 90 days with no activity.

@github-actions github-actions bot added the Stale label Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants