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

EAPI=8 support #32

Open
22 tasks done
Ionic opened this issue Aug 27, 2021 · 40 comments
Open
22 tasks done

EAPI=8 support #32

Ionic opened this issue Aug 27, 2021 · 40 comments
Labels

Comments

@Ionic
Copy link
Collaborator

Ionic commented Aug 27, 2021

Every few years, paludis breaks.

This is one of those times.

EAPI 8 has been released and is getting used now, which means that paludis will just silently ignore those ebuilds.

As usual, mgorny released an ultimate guide to EAPI 8.

The changes look mostly managable, although the IDEPEND feature needs invasive work similar to the BDEPEND feature (and even that one was never properly implemented to take cross compiling into account, rather merged with normal dependencies).

Checklist:

  • bash, requiring at least 5.0, compat50
  • --datarootdir automatically passed by econf
  • --disable-static automatically passed by econf
  • doconfd not influenced by insopts
  • doenvd not influenced by insopts
  • doheader not influenced by insopts
  • doinitd not influenced by exeopts
  • dosym -r for relative symlinks
  • fetch+ notation in SRC_URI to override fetch restriction
  • mirror+ notation in SRC_URI to override mirror restriction
  • hasq banned
  • hasv banned
  • useq banned
  • IDEPEND as host-based-only post-install-time dependency variable, mimicking what BDEPEND is for DEPEND - can probably be merged into RDEPEND
  • PATCHES does not allow options any longer
  • PROPERTIES is append by default instead of overwrite
  • RESTRICT is append by default instead of overwrite
  • test_network as new PROPERTIES value for network support during the test phase
  • unpack dropped support for 7-zip, LHA and RAR formats
  • updates filenames need no longer follow the nQ-yyyy scheme
  • usev accepts optional second parameter to override output value
  • pkg_* phases set working directory to an empty (but existing) directory
@negril
Copy link
Collaborator

negril commented Dec 3, 2021

Does anyone know how to properly set up a cross compiling environment? Do I just extract a CBUILD stage3 somewhere and point BROOT to it?

@Ionic
Copy link
Collaborator Author

Ionic commented Dec 3, 2021

You cannot do this with paludis (yet, until somebody implements it in a proper way).

BROOT is not supported, although it's part of EAPI=7. We just never implemented that. Even host-based build dependencies are just a hack/shim in paludis that get just merged into the general build dependencies.

In the bad old days, crossdev was the way to go, together with custom repositories and a magic bashrc.

This issue report is about EAPI=8 support, for which this question... really is inappropriate.

@negril
Copy link
Collaborator

negril commented Dec 3, 2021

You might want to reread the question again. ;)

I know it is not implemented, but the --host-root from EAPI=5 is and you can implement the new has_version and best_version parameters using the same apis. Hence why I asked how I would set up the environment so I can implement the tests for it.

@Ionic
Copy link
Collaborator Author

Ionic commented Dec 13, 2021

Ah, in that case, yes, I think that should do the trick. It's just that BROOT won't work.

In other news, I've started implementing EAPI 8 stuff, privately for now. Will update the original post accordingly by ticking off items that are done. I'm lazy on test cases, though, so any work publishing will probably be slow, since new features are useless without proper test cases. More to come.

@negril
Copy link
Collaborator

negril commented Dec 16, 2021

Did you base your work on the current master from exherbo? There are some fixes in there that would be nice to have. Especially in regards to cmake and python.

@Ionic
Copy link
Collaborator Author

Ionic commented Dec 16, 2021

Yep. I've always rebased the EAPI 7 stuff against exherbo's master for my personal use for the past few years. Same thing now, especially in order to fix the python 3.9 breakage. I'd like to uplift this repo, too. But I'm not quite done yet. :)

@Ionic
Copy link
Collaborator Author

Ionic commented Dec 16, 2021

Take a look at my fork, I've just rebased against exherbo's master for fun. Don't expect EAPI 8 changes just yet, I haven't put them in there yet.

That was the uplifting I've been talking about. Hopefully I didn't mess it up somewhere.

@Ionic
Copy link
Collaborator Author

Ionic commented Dec 18, 2021

I've pushed my changes to the feature/eapi8 branch, but do not try to use any of this until it's fully done. I've also added a warning to README.md, but I have to reiterate that using the current state will lead to missed dependencies.

However, for those curious, take a look.

@MageSlayer
Copy link
Owner

@Ionic
Thanks a lot, you are a real driver for this issue.

@Ionic
Copy link
Collaborator Author

Ionic commented Jan 6, 2022

As an update on this issue, my current branch (see above) now contains a feature-complete EAPI=8 implementation (with the usual caveat on IDEPEND and cross-compiling/cross-architecture support, which is still missing in paludis).

It's missing a lot of test cases for the new features, though, so it's not prime-time or merge ready.

I'm confident enough to use it on my own system and update my packages with it, but will have to add test cases for each new feature to make sure that it really works correctly.

At the very least, I know that the already existing tests don't fail, so I haven't introduced regressions / EAPI=7 and lower ebuilds should continue to work as they always have.

@MageSlayer
Copy link
Owner

MageSlayer commented Jan 30, 2022

I've installed your branch (without Python support) is my system & managed to update quite a lot of EAPI8 packages in automatic fashion. Almost everything went fine. Your work is impressive & definitely useful.
Please keep on.

@Ionic
Copy link
Collaborator Author

Ionic commented Jan 31, 2022

We really need EAPI=8 support badly, but I need to add test cases for the new things I implemented. I'll keep working on that.

I'm pretty sure that everything on my branch should work correctly, but making sure that it does doesn't hurt.

I've only seen a few issues with specific ebuilds, some of which are general ones going down to EAPI=0 (for instance #40). Others, like games-emulation/dolphin-5.0_p20210506-r3, for which I haven't created an issue report yet, require to dig into PMS or request guidance from the Gentoo portage team, but overall, my upgrade has been smooth (except in cases were ebuilds were really broken with all conforming package managers).

@thesamesam
Copy link

(As usual, please let me know if there's something on the Gentoo side that you need, be it fixed ebuilds, or something else.)

@Ionic
Copy link
Collaborator Author

Ionic commented Jan 31, 2022

Not immediately, I really have to look it up first. :)

The dolphin ebuild utilizes a somewhat global variable in src_prepare without explicitly calling it global or exporting it. That seems to work with portage, but fails with paludis, since variables not explicitly maked as global in a top-level context are not useable within phases with Paludis. Fixing that is trivial, i.e., use declare -Ag instead of declare -A to force it being global (since, otherwise, the scope will be local to the function that sources the ebuild, but not passed down to the actual phase invocations). My gut feeling is that Portage is actually doing the right thing here, since variables declared in a global context, i.e., not within a function, should be global anyway. I'd have to read through PMS to confirm that, though.

Then, there was dev-libs/mpfr-4.1.0_p13-r1, which ironically was last touched by you. This ebuild generates a patch list to fetch in a sophisticated way and then tries to apply them all from ${DISTDIR}. Portage seems to copy distfiles into a private, package-related directory and sets ${DISTDIR} to this directory, while for Paludis, it's always the global distfiles directory. This subtle difference let the mpfr ebuild try to apply patch files like android-tools-31.0.3-disable-werror-boringssl.patch (since I installed/upgraded android-tools some time before mpfr). Likewise, the fix for this is a trivial change to PATCHES+=( "${DISTDIR}/${MY_P}-patch"*.patch ). Portage's behavior of copying all needed files in ${A} does make sense, even if just to keep them neatly organized while building a package, but I'm not convinced that the additional copy overhead (even if it can be omitted via hardlinks when supported) is worth the effort.

@negril
Copy link
Collaborator

negril commented Feb 1, 2022

The test cases should now be more or less complete and under Tests
These are the tests that currently fail:

ERepository7:

  • changed_domo
  • added_dostrip
  • econf_added_options
  • eend_not_to_stdout
  • banned_libopts
  • prefix
  • changed_vars
  • best_version (should fail but is worked around atm)
  • has_version (should fail but is worked around atm)

ERepository8:

  • dosym_rel ( resulting link has the wrong target ../foo instead of foo)
  • restrict_mirror (fails to override restrictions)
  • restrict_fetch (fails to override restrictions)
  • banned_functions (banned functions are still defined, use unset and rewrite code to use has instead of hasq)
  • test_network (tests are never run because we don't check ALLOW_TESTS for network)

depend_idepend (missing execute permissions on *_{setup,clean}.sh, can by fixed by setting those or prepending bash in run_tests.sh)

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 1, 2022

Thank you very much!

I'll cherry-pick some of your changes and apply them to my feature/eapi8 branch, mainly the test cases themselves. I'll also fix the stupid executable permission error I made directly...

A few observations:

  • do we really need C++17 support yet? If possible, I'd keep that synced up with the main paludis code base. I'm pretty sure that upstream will eventually switch to C++17 anyway, but until they do, since changing that can introduce subtle behavioral changes in the whole code base, that we might be unaware of.
  • switching to gtest_discover_tests doesn't sound like a bad idea, especially since debugging tests is a pain with the current setup, but do we actually get anything else out of it? Gtest already seems to run tests in parallel if possible (at least that was my impression since I've seen it starting multiple tests which only terminated at later times, depending on how complex they are), even without gtest_discover_tests, and currently, the environments will differ between the two test runs, which is something to be avoided

ERepository8:
* [...]
* test_network (tests are never run because we don't check ALLOW_TESTS for network)

ALLOW_TESTS is a portage-specific variable, as far as I know, we don't have that. Paludis has different means of enabling or disabling tests, mainly via the optional_tests and expensive_tests BUILD_OPTIONS.

Again, thank you, I'll get working on hooking that up.

@negril
Copy link
Collaborator

negril commented Feb 2, 2022

The switch to C++17 isn't really necessary per se, but I'd really prefer refactoring FSPath on top of std::filesytem. I can add an old fashioned implementation so we can use a use flag. I'll ask the paludis guys about their plans going to C++17.

gtest_discover_tests calls the test binary with --gtest_list_tests. gtest_add_tests scans source code. And we really defeat that by hiding everything behind the run_test.sh so you will get a single test out of e.g. e_repository_TEST_8.cc. The way I implemented it you get 15. Which you can all start on their own or in parallel, and you can debug every single one of them on their own.

The reason the environments differ is because I commented out variable definitions that are never used in the C++ tests.
The env -i call in the old tests is kinda counter productive since the ebuild tests will also run with a polluted environment. You can run the new tests using env -i ctest <...> getting the same result. Eventually I'd like the change over all the tests to the support gtest_discover_tests starting with the C++ tests. On my testing branch e_repository_TEST_6 fails and I have no clue where and debugging it is a huge pita. Once I figure out how the python and ruby tests work I am certain we can emulate --gtest_list_tests for them as well. So we end up having finer granulated testing from the change. After that gtest_add_tests can disappear and we unify it again.

expensive_tests is exheres only and I set optional_tests. The issue is, we see the RESTRICT="test" and don't test but have no way to override it as portage has with ALLOW_TESTS so either we do it by default or add a way.

edit: talked to heirecka in #paludis on LibraIRC, he didn't see a particular reason not to raise it, so I will a c++-version detection and add an old implementation as well.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 3, 2022

Don't add old implementations, I'm already doing that and fixing what I can find. If they switch to requiring C++17, we can just apply your changes.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 3, 2022

ERepository8:
[...]
* banned_functions (banned functions are still defined, use unset and rewrite code to use has instead of hasq)

That seems to be fine. PMS defines banned commands as this:

Some commands are banned in some EAPIs. If a banned command is called, the package manager must abort the build process indicating an error.

It doesn't say that the commands must not be defined. Providing a wrapper which aborts the build process, which is what we do, is absolutely aligned with the spec.

@negril
Copy link
Collaborator

negril commented Feb 4, 2022

Do you want me to change the tests?

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 4, 2022

No, don't worry. I'm currently doing that, fixing tests that are wonky and improving them. I'll just need a few more days to get it all done. :)

@negril
Copy link
Collaborator

negril commented Feb 5, 2022

@Ionic there are 3 more commits in https://github.com/negril/paludis/commits/wip/feature/tests after the one you cherry picked from. Some of the cleanups in there you haven't done yet. And the restriction tests actually work now.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 6, 2022

Oh, right, I just pushed my changes yesterday in a hurry. Didn't even test them yet.

I'll get the other changes, too... and actually test them. Note that the restriction label override test shouldn't have worked, so there's something wonky with it. The first label in there is "file+", which shouldn't be a recognized one... and actually cause the test to fail.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 6, 2022

I think that I have applied all your changes now, with a few exceptions:

  • I've omitted things that look like pure debugging output, like a lone "find" command.
  • fixed up the "file+" prefix by switching it to "fetch+".
  • kept the BASH_COMPAT test as-is, since EAPIs are requiring exactly one specific compat value - newer ones aren't generally allowed and compat values aren't bumped up within an EAPI (the occurrednces we saw were actually caused by eclasses or ebuilds changing things, if I remember correctly... or something else not being applied correctly).
  • kept the dosym test as-is, since I've rewritten it anyway (also, your code still looks broken, since readlink on /usr/bin/foo doesn't sound like a good idea, not sure why you dropped ${D}).
  • omitted the ALLOW_TEST env setting, since it's portage-only and won't help us.

I'll try to rework the override labels tests again tomorrow, since something is definitely wonky if they succeed when they shouldn't. Additionally, this stuff is so complicated that we actually want to also make sure that the whole testing matrix works and we're missing a few cases.

After that's done, I'll actually try to build all of this stuff and see how it goes.

@negril
Copy link
Collaborator

negril commented Feb 6, 2022

Note that the restriction label override test shouldn't have worked, so there's something wonky with it. The first label in there is "file+", which shouldn't be a recognized one... and actually cause the test to fail.

In /paludis/fetchers is a script called dofile that never gets installed. In the environment for the tests we set PALUDIS_FETCHERS_DIR so in ebuild.cc the file:// fetcher gets picked up. This requires the echo_functions to work afair, hence the changes in c183e87. The files in SRC_URI then get correctly fetched from the mirror directory (or should be).

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 7, 2022

Thanks for the explanation, but that's not what I meant.

Your current code has a typo where one of the files is prefixed with file+file:// instead of fetch+file://, which means that the test should have failed to fetch test2.tar.xz.

At least in theory. Practically, I guess that the distfile was just discarded since file+file:// is not a known URL scheme. If that's true, than fetching is pretty lenient. I would have made unrecognized URLs an error, instead of just skipping them. I'll probably want to keep that in mind and see how, e.g., portage handles this. PMS doesn't specify what should happen with unparseable URLs.

Edit: besides the failure due to the typo, it should have failed anyway, since the non-prefixed distfile test1.tar.xz shouldn't be fetchable, no matter what the other two are set up to. So if this test worked, it's doubly wrong in doing so.

I'll debug this and find out what's wrong. My initial suspicion is that this happens due to distfile caching in distdir.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 7, 2022

Okay, with the modified test suite, I'm seeing the following failures:

  • EAPI=7
    • domo still influenced by into (was never modified not to do this)
    • dostrip completely missing
    • eapply_user git-diff support (probably caused by my code, will need to evaluate if PALUDIS_USER_PATCHES is set and passed correctly, can be ignored for now)
    • --target=${CTARGET} and --with-sysroot=${ESYSROOT:-/} missing from econf calls (last one is true, first one isn't, test needs fix up and some debugging, so can also be ignored for now)
    • eend writes to stdout
    • eqawarn not defined (that's actually quite common, I've seen this happening for a lot of ebuilds with paludis, it's completely missing an implementation)
    • ... internal exception, test stopped, needs fixup. Will do so later.
  • EAPI=8
    • BASH_COMPAT is 5.1, not 5.0 (what? Why? Needs debugging.)
    • dosym -r failed (as you've mentioned, ../foo vs. foo)
    • restrict-none fails (doesn't strip the override labels)
    • restrict-mirror fails (same)
    • restrict-fetch-alllabels fails (overrides not applying for test{2,3}.tar.xz, needs debugging and fixing)
    • hasv not banned (needs proper hook ups in CMake, TBD, easy fix)
    • accumulated PROPERTIES/RESTRICT fails (? needs debugging)
    • test_network not honored
    • second argument to usev not supported (needs debugging)
    • pkg_* phases don't start in empty directory (needs debugging)

I'll continue working on this soon.

@negril
Copy link
Collaborator

negril commented Feb 7, 2022

I had implemented eqawarn in negril@2bebb03.

From the PMS:

bash-version The ebuild file format is in its basic form a subset of the format of a bash script. The interpreter is assumed to be GNU bash, version as listed in table 6.1, or any later version. If possible, the package manager should set the shell’s compatibility level to the exact version specified. It must ensure that any such compatibility settings (e. g. the BASH_COMPAT variable) are not exported to external programs.

Hence BASH_COMPAT is the minimum bash version required and the reason I used ver_test for the test case.

If you want to check if the option is set correctly we would have to test PALUDIS_BASH_COMPAT.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 8, 2022

I've fixed a few things, mostly in the EAPI=7 range, but not everything yet.

Thanks for the eqawarn work, I've picked that now.

Other than that, I added --with-sysroot to econf for EAPI >= 7, fixed the econf test due to a stupid typo on my end, fixed the vers_* test on my end (your code wasn't broken), inverted the failure logic for selectors-dep (we expect a failure there, not success), inverted the failure logic for selectors-or (since EAPI=7 is actually supposed to fail with -a-b+foo+bar) and added more tests for that to test other combinations, same for selectors-xor.

The eapply_user failure was my fault, as already expected.

Tomorrow I'd like to work on some low-hanging fruits like the eend/eqawarn to stdout issues.

@negril
Copy link
Collaborator

negril commented Feb 11, 2022

@Ionic don't put to much work into fixing the selector test fails. They work fine ( minus the still matching empty sets ) with the reworked test environment.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 11, 2022

The issue with the selectors test has merely been PretendAction objects never resetting their state once a failure has occurred. I've fixed that already.

Most of the new selectors tests I've written now work correctly, but we certainly have an issue, since I never implemented the new "empty OR/XOR group" behavior that is now required with EAPI=8, so these failures are totally sane and helping us out a lot.

I've been fixing a lot of issues in the past few days (also for EAPI=8, although I haven't force-pushed this branch yet, since I've had to rebase often anyway due to the EAPI=7 changes), most implemented things now work correctly. I'm still debugging the test_network stuff, but should eventually figure out what's going on there.

Other than that, your test cases have been immensely helpful, especially for EAPI=7. I didn't even realize that there are things we haven't implemented correctly or at all yet. dostrip is one such example. We also miss a lot of *DESTTREE changes etc. I'll update this issue after the test_network test actually works and will give a brief summary what fails.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 12, 2022

The BASH_COMPAT failure is really a bug in bash. I've reported this upstream and worked it around in paludis.

Other than that, the good news is that all EAPI=8 test cases are now succeeding after all the fixes I've made.

The following EAPI=7 test cases are still failing:

  • domo uses into
  • dostrip unimplemented
  • prefix unimplemented
  • DESTTREE still exposed
  • selectors-dep due to unimplemented empty OR group behavior change
  • selectors-or due to unimplemented empty OR group behavior change
  • selectors-xor due to unimplemented empty XOR group behavior change

Thus, the only failing tests are due to unimplemented functionality, and, especially, only in EAPI=7. I'll continue working on that.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 18, 2022

I've since implemented (for EAPI=7+):

  • domo not using into
  • {,INS}DESTTREE not exposed any longer, instead redirected to internal variables called PALUDIS_INTERNAL_{,INS}DESTTREE
  • dostrip

Afterwards, I noticed that we don't have proper ENV_UNSET support, which is new for EAPI=7 and set in the profile. Initially, I thought that implementing this would be easy, but it turns out that it's a lot more complicated, mainly because it's a profile variable defined in make.defaults, which, additionally, also has to be handled in an incremental fashion (i.e., the variable stacks between parent and child profiles and the special syntax known from other places, like -* to remove all previous values and -... to remove a specific value must be handled).

That alone isn't a big deal, since Paludis has proper support for this internally, but incremental profile variables are, so far, not exported back to the ebuild environment. Instead, ebuild.bash starts out with a specific set of environment variables set (mostly internal PALUDIS_... variables and a few common ones like P, PV, PN etc.) and everything else is added or removed by sourcing/handling profiles and the ebuild itself. Importantly, while ebuild.bash does support recursively defined profiles, the simple bash code supports none of the fancy features like incremental variables or USE_EXPAND logic.

Additionally, the code that cleans up the environment with a fixed list of variables that are supposed to be unset, which should be extended by ENV_UNSET eventually, executes before any of the profile sourcing is done, so ENV_UNSET isn't even available there.

While I can easily refactor this to clean up variables in ENV_UNSET only after sourcing the profile, the issue/bug with incremental variables is an important one and requires additional work.

More to come.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 19, 2022

That alone isn't a big deal, since Paludis has proper support for this internally, but incremental profile variables are, so far, not exported back to the ebuild environment. Instead, ebuild.bash starts out with a specific set of environment variables set (mostly internal PALUDIS_... variables and a few common ones like P, PV, PN etc.) and everything else is added or removed by sourcing/handling profiles and the ebuild itself. Importantly, while ebuild.bash does support recursively defined profiles, the simple bash code supports none of the fancy features like incremental variables or USE_EXPAND logic.

Luckily, my analysis wasn't quite right. While there are a few variables which are not correctly exported, most of them seem to be, namely USE, USE_EXPAND, USE_EXPAND_HIDDEN, CONFIG_PROTECT and CONFIG_PROTECT_MASK. These are passed through a special mechanism in ebuild.cc and the do_..._action.cc files.

However, other variables (which I've looked up first), are missing from this special handling, namely IUSE_IMPLICIT, USE_EXPAND_IMPLICIT and USE_EXPAND_UNPREFIXED. Since PMS does not explicitly specify that these variables are conceptual variables only which are not supposed to be exported, I guess we should do that in Paludis. I'll fix that up quickly.

Implementing ENV_UNSET is done, so now we're missing:

  • BROOT
  • prefix
  • empty ... group behavior.

@negril
Copy link
Collaborator

negril commented Feb 19, 2022

I've implemented the empty group changes. I'll upload them tomorrow. The cross build changes aren't that hard, but we need to decide how we want to configure that. AFAIK portage just uses just BROOT and decides from that. But I think we might be able to solve it via installed repositories.

e: exherbo has a cross branch we might look at that as well.

@negril
Copy link
Collaborator

negril commented Feb 20, 2022

Changes are at Ionic#1.

@negril
Copy link
Collaborator

negril commented Feb 20, 2022

While testing I ran into the realisation that we wrongly assume that the top node of a dependency is a all-of group.
Due to that a REQUIRED_USE="python? ( ${PYTHON_REQUIRED_USE} ) ruby? ( ruby_targets_ruby${RUBY_VER/./} )" like we have in the paludis ebuild will not match with -python and/or -ruby.

We'll need a new root element and implement it everywhere. I'll report back when I'm done.

@Ionic
Copy link
Collaborator Author

Ionic commented Feb 20, 2022

Thanks a lot for that! I'll look into it at a later time.

While testing I ran into the realisation that we wrongly assume that the top node of a dependency is a all-of group.
Due to that a REQUIRED_USE="python? ( ${PYTHON_REQUIRED_USE} ) ruby? ( ruby_targets_ruby${RUBY_VER/./} )" like we have in the paludis ebuild will not match with -python and/or -ruby.

Isn't that okay? I mean, the root being an all-of group is fine, since setups such as -ruby -python will lead to both statements being removed during flattening and an empty all-of group is trivially true, or am I missing something?


I got a bit side-tracked by something I read in PMS, namely:

For EAPIs listed in table 5.4 as supporting profile defined IUSE injection, the variables named in USE_EXPAND and USE_EXPAND_UNPREFIXED shall have their profile-provided values reduced to contain only those values that are present in IUSE_EFFECTIVE.

A setup such as

=== parent ===
ARCH="cheese"
USERLAND="GNU"
LINGUAS="enabled_en enabled_en_GB enabled_en_GB@UTF-8"
WEIRDVAR="water ski"
OTHERVAR="banana ray"
USE_EXPAND="LINGUAS USERLAND WEIRDVAR"
USE_EXPAND_UNPREFIXED="ARCH"
USE_EXPAND_IMPLICIT="USERLAND ARCH"
USE_EXPAND_VALUES_USERLAND="GNU"
IUSE_IMPLICIT="build"
=== child ===
USE_EXPAND_UNPREFIXED='OTHERVAR'
USE_EXPAND='-WEIRDVAR'
WEIRDVAR='sand storm'
OTHERVAR='death'

leads to some interesting setup in pkg_pretend:

declare -x USE_EXPAND="LINGUAS USERLAND"
declare -x USE_EXPAND_HIDDEN=""
declare -x USE_EXPAND_IMPLICIT="USERLAND ARCH"
declare -x USE_EXPAND_UNPREFIXED="OTHERVAR"
declare -x USE_EXPAND_VALUES_ARCH="cheese otherarch"
declare -x USE_EXPAND_VALUES_USERLAND="GNU"
declare -x USE_EXPAND="LINGUAS USERLAND"
declare -x IUSE_IMPLICIT="build"
declare -x USE_EXPAND_IMPLICIT="USERLAND ARCH"
declare -x USE_EXPAND_UNPREFIXED="OTHERVAR"
declare -x IUSE_EFFECTIVE="build cheese otherarch userland_GNU"
declare -x ARCH="cheese"
declare -x WEIRDVAR="sand storm"
declare -x OTHERVAR="death"

This seems to be wrong for multiple reasons. First, since IUSE_EFFECTIVE does not include any values from OTHERVAR, but USE_EXPAND_UNPREFIXED includes OTHERVAR, if I understand PMS correctly, OTHERVAR should be reduced to being empty.

Secondly, I may misunderstand PMS again, but USE_EXPAND (and by extension USE_EXPAND_UNPREFIXED is described as:

Defines a list of variables which are to be treated incrementally and whose contents are to be expanded into the USE variable as passed to ebuilds.

which, to me, means, that any variables that are part of USE_EXPAND{,_UNPREFIXED} must be also treated incrementally, which is not what is happening here. Or maybe the incremental part is only relevant for the USE_EXPAND{,_UNPREFIXED} variables themselves, which is already documented at a different location.

I'll check how portage behaves to have a baseline comparison.

Edit: portage does this:

declare -x USE_EXPAND="LINGUAS USERLAND"
declare -x USE_EXPAND_IMPLICIT="ARCH USERLAND"
declare -x USE_EXPAND_UNPREFIXED="ARCH OTHERVAR"
declare -x USE_EXPAND_VALUES_ARCH="cheese otherarch"
declare -x USE_EXPAND_VALUES_USERLAND="GNU"
declare -x USE_EXPAND="LINGUAS USERLAND"
declare -x IUSE_IMPLICIT="build"
declare -x USE_EXPAND_IMPLICIT="ARCH USERLAND"
declare -x USE_EXPAND_UNPREFIXED="ARCH OTHERVAR"
declare -x IUSE_EFFECTIVE="build cheese otherarch userland_GNU"
declare -x ARCH="cheese"
declare -x WEIRDVAR="sand storm"
declare -x OTHERVAR=""

That seems to be in line with the PMS specification (at least when it comes to reducing the content of variables), so, meh, paludis really does not seem to calculate this correctly. Which is quite surprising, given that Ciaran wrote the PMS section.

But, even though that's more in line with PMS, modyfing child_profile by adding USE_EXPAND_IMPLICIT='OTHERVAR' creates a rather surprising result with portage:

declare -x USE_EXPAND="LINGUAS USERLAND"  
declare -x USE_EXPAND_IMPLICIT="ARCH OTHERVAR USERLAND"
declare -x USE_EXPAND_UNPREFIXED="ARCH OTHERVAR"
declare -x USE_EXPAND_VALUES_ARCH="cheese otherarch" 
declare -x USE_EXPAND_VALUES_USERLAND="GNU"
declare -x USE_EXPAND="LINGUAS USERLAND"
declare -x IUSE_IMPLICIT="build"
declare -x USE_EXPAND_IMPLICIT="ARCH OTHERVAR USERLAND"
declare -x USE_EXPAND_UNPREFIXED="ARCH OTHERVAR"
declare -x IUSE_EFFECTIVE="build cheese otherarch userland_GNU"
declare -x ARCH="cheese"
declare -x WEIRDVAR="sand storm"
declare -x OTHERVAR=""

Here, OTHERVAR should be set to banana ray death, since OTHERVAR is part of both USE_EXPAND_IMPLICIT and USE_EXPAND_UNPREFIXED (at least within the child profile) and its whole content should be part of IUSE_EFFECTIVE, but yet, it's empty.

So either I misunderstood something or both PMs behave weirdly.

I'll probably ask for help at a later time, but also likely won't be able to continue working on it this before the start of March.

@negril
Copy link
Collaborator

negril commented Mar 16, 2022

While testing I ran into the realisation that we wrongly assume that the top node of a dependency is a all-of group.
Due to that a REQUIRED_USE="python? ( ${PYTHON_REQUIRED_USE} ) ruby? ( ruby_targets_ruby${RUBY_VER/./} )" like we have in the paludis ebuild will not match with -python and/or -ruby.

Isn't that okay? I mean, the root being an all-of group is fine, since setups such as -ruby -python will lead to both statements being removed during flattening and an empty all-of group is trivially true, or am I missing something?

Initially I misread the truth table for a? ( b ), assuming it would return false if a doesn't match. But it is an implication so it only returns false if a matches and b does not. From this I wrongly assumed the all group would still require the conditional to match. It's never touched by flattening though, that only happens when writing to the VDB.

The assumption that it's an all-of group is still weird to me because we treat it differently at least in the required_use_verifier.cc.

By now I've fixed the empty group changes and pushed them have a look at that please because I might have went overboard with the test case. :)

In other news, we need to have a look at the resolver. For me removing python_targets_python3_8 and leaving python_targets_python3_9 from dev-python/pyparsing leads to breaking app-emulation/spice. But pyparsing is only in bdepend and the at-least-one-of group is still satisfied with python3_9. This shouldn't break. But the decider only adds dev-lang/python as a dependent. I haven't really made progress there because debugging that without any pretty printers or debugging methods is a pita and time consuming.

gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Mar 16, 2022
There's no guarantee that ${DISTDIR} only contains the distfiles
for this package (it could be the system's whole cache, for example --
like in Paludis).

Bug: MageSlayer/paludis-gentoo-patches#32 (comment)
Thanks-to: Ionen Wolkens <[email protected]>
Signed-off-by: Sam James <[email protected]>
@Ionic Ionic added the eapi8 label Oct 10, 2023
@Ionic
Copy link
Collaborator Author

Ionic commented Oct 10, 2023

Linking the eapi8 branch to this bug report.

While it isn't prime-time-ready yet, it includes most features and will be "stable" (as in: no rebases, not bug free or feature complete) going forward, unlike the gentoo/* branches that we will keep rebasing on top of upstream's branches.

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

When branches are created from issues, their pull requests are automatically linked.

4 participants