-
Notifications
You must be signed in to change notification settings - Fork 27
Buildroot for eln-extras #83
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
Conversation
66571a6
to
be06ae0
Compare
I have ran tests and it looks like things are working. |
Looking at the results you uploaded, a few issues are apparent:
|
be06ae0
to
cb605b1
Compare
I just ran a full test and I am not seeing point 2 happening. I am seeing AppStream, BaseOS and CRB packages in the package list. I think I know how to get those out. I'm also seeing packages in the "buildroot" repo. Most of those are related to ghc. I believe these are possibly ELN packages that are no longer in ELN. I'm not going to touch those with this code, at least not on this pass. |
Looking at the "before" views, it seems this "Listing packages in ELN" has nothing to do with this patch. |
There is a difference. Yes, currently some source packages which are in ELN are also listed in ELN Extras, but only when the binary packages listed in Extras are NOT listed in the ELN runtime set (required+deps, not including builddeps). Some of these might be shipped in BaseOS/AppStream/CRB, but remember that CR doesn't really distinguish by repo. One example of this is git-email (subpackage of git), where git and some of its subpackages are listed, but git-email is not. This is simply a matter of adding to the configs to shift them over to the ELN list (if they are supposed to be shipped in RHEL). It's been a while since last time, but IIRC there were binary packages in the Extras results that were already listed in the ELN runtime set, and that was the regression. There is a part of the code that essentially says "if this RPM is listed in the base config list, exclude it from the add-on list", and that wasn't seeming to happen after the first patch. Could you please upload the results of your latest test so that I can review them? |
I guess I'm not seeing what you are seeing. |
To demonstrate point 1: If you run the following, this shows the RPMs (binary packages) listed in both the ELN runtime set and ELN Extras:
Generally, that would return no overlaps due to the deduplication code in CR; the only things listed there are very specific, e.g. packages shipped on only some arches in ELN but are used on all arches in Extras (e.g. python-colorama) or packages which would otherwise not be listed in the runtime but are runtime deps of placeholders (e.g. socat). However, when I make the same comparison against these results, there's a TON of overlap:
zlib-ng is just one example of packages which are required in ELN and listed again in ELN Extras. This indicates that the deduplication isn't happening with the build dep detection. |
I believe the ELN packages showing up in Extra's is due to a race condition. Doing the buildroot for Extra's is just showing up the problem more. Here is where the analyzer checks to see if a package is already in the list of ELN (base_vew) packages. This is in the _analyze_view function, which get's called by the _analyze_views function. _analyze_views does the base_view first, which is great. Except that it does it just once per pass. _analyze_views get's called by each pass here So, here's the real problem. If a package is in extras on the first pass, but not in ELN on the first pass, it get's put in the extras list. There is no second check. So even if the package get's put on ELN on the second pass, it still stays on the extras list. But again, this has nothing to do with this pull request. It's just that it's showing up more. |
If you grab files from the currently Content Resolver: and compare them then you see that we have the following duplicate packages both in Extras and ELN I guess those are supposed to be there. |
Looking at your comment, you are comparing two different files, one with all, one without. If you do them both all, you will see this.
|
This is on purpose. The assumption (whether correct or not) is that packages that are only buildroot deps (iow not listed in the runtime set which is required+deps) will not be shipped, therefore having them as dependencies in an add-on (which could only depend on the shipped repos of the base) will need to somehow accommodate the lack of availability of the dependency. While it's not 100% accurate since CR configs aren't completely thorough, it mostly works right now where we're just looking at runtime deps of Extras. (It also highlights a few packages where we can improve the composes, e.g. python-colorama as recently discussed). Granted, once we start looking at buildroot deps of Extras, there's going to be more overlaps because CRB isn't well accounted for in ELN configs. Some of that is just indicating where we can make the ELN configs more thorough, and perhaps some further changes will be necessary to better handle these. But, as a simple example, all the zlib-ng-compat subpackages are listed as Required in ELN, and if deduplication were working, it shouldn't show up in Extras; the fact that it is still listed would indicate that it's not working. |
I'll be honest. I really don't know what you want. I don't even know why you are concered. Does this affect ELN going to RHEL 11? No. To me, this seems like a feature, not a bug. |
I have created the feature request for removing eln packages from eln-extras view. |
Because this feature, more than doubles the processing time of Content Resolver, I am recommending this does not get merged until this other feature is implemented that should cut down the time. |
After offline discussion with Yaakov, it was decided to merge this today (Monday Sep. 22) so we could see what the results would look like after a run or three. |
Revert "Merge pull request #83 from tdawson/buildroot-extras"
This adds buildroot functionality to eln-extras
It needs a bit of cleaning up (squashing), and at least one more test, but it works.