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

Do not export PMS variables in EAPI 9 #1407

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Flowdalic
Copy link
Member

Instead of passing PMS variables as part of the process environment, we pass them via a file that is later sourced by the ebuild's bash.

Since, for example, A is usually the greatest contributor to the process environment, removing it from the process environment significantly avoids running into MAX_ARG_STRLEN when spawning a new child process.

This means that A and other PMS variables are no longer exported in the ebuild and hence unavaiable to child processes. However, A is mostly used as part of the default_src_unpack function and there A does not need to be exported.

This started as a change that only unexported A, but was later extended to most PMS variables as suggested by ulm in https://bugs.gentoo.org/721088#c23.

Thanks to Zac Medico for helpful input on this change.

Closes: https://bugs.gentoo.org/721088

@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch from 13610de to 6753aea Compare December 30, 2024 15:44
@Flowdalic Flowdalic changed the title WIP: Do not export PMS variables in EAPI 9 Do not export PMS variables in EAPI 9 Dec 30, 2024
@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch 5 times, most recently from 879a6fb to 67106ed Compare December 30, 2024 18:40
lib/portage/const.py Outdated Show resolved Hide resolved
@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch 2 times, most recently from 4ba182a to a8e44e3 Compare December 31, 2024 09:20
@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch from a8e44e3 to 834f78c Compare January 1, 2025 14:24
Flowdalic added a commit to Flowdalic/gentoo that referenced this pull request Jan 1, 2025
Once a future EAPI does no longer export PMS variables (bug #721088 and
[1]), we need to explicitly ensure that those are exported to helper
commands which expect certain PMS variables in their process
environment.

While the portage ebuild is usually deliberately not using the latest
EAPI, it may be a good idea to make it explicit that the _compat_upgrade
helpers expect ED in their process environment.

1: gentoo/portage#1407

Signed-off-by: Florian Schmaus <[email protected]>
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Jan 1, 2025
Once a future EAPI does no longer export PMS variables (bug #721088 and
[1]), we need to explicitly ensure that those are exported to helper
commands which expect certain PMS variables in their process
environment.

While the portage ebuild is usually deliberately not using the latest
EAPI, it may be a good idea to make it explicit that the _compat_upgrade
helpers expect ED in their process environment.

1: gentoo/portage#1407

Signed-off-by: Florian Schmaus <[email protected]>
Closes: #39934
Signed-off-by: Sam James <[email protected]>
@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch 2 times, most recently from bed117b to 7ef309a Compare January 7, 2025 13:19
@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch 5 times, most recently from 5562b8e to ba9955e Compare January 8, 2025 18:37
@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch 4 times, most recently from c3264d6 to b4201dd Compare January 9, 2025 10:18
@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch 3 times, most recently from 6e5e977 to 4f097cd Compare January 9, 2025 12:35
# exists if we are processing a phase that is in
# _phase_func_map. But better safe than sorry.
if mysettings.get("EBUILD_PHASE") in _phase_func_map.keys() and os.path.isdir(
t
Copy link
Member

Choose a reason for hiding this comment

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

We could probably make this just mysettings.get("EBUILD_PHASE") != "depend".

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered mysettings.get("EBUILD_PHASE") != "depend". The drawback is that, as soon as portage adds another phase with similar constraints like "depend", the code would need to be adjusted or otherwise the if-condition would erroneously hold. Whereas, if mysettings.get("EBUILD_PHASE") in _phase_func_map.keys() is true, then we know with the current implementation, that T will exists (since it is mandated for all PMS ebuild phase functions to exist) and this is also likely to be true for potential future PMS ebuild phase functions (simply because T is useful all the time).

That said, this was just explaining the rationale why I did it this way. I ultimately leave the decision to you, because you have far more experience with portage. Let me know if it should stay as it currently is, or if you want me to change it to mysettings.get("EBUILD_PHASE") != "depend".

Copy link
Member

Choose a reason for hiding this comment

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

I think it's nicer to use mysettings.get("EBUILD_PHASE") != "depend" since we only implement the temp file cleanup for the depend phase. If we add another phase with similar constraints then it's easy enough to add a special case for it here.

Copy link
Member Author

@Flowdalic Flowdalic Jan 11, 2025

Choose a reason for hiding this comment

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

since we only implement the temp file cleanup for the depend phase.

Then we seem to have a general problem. I've added a debug print to emit the phase names we are seeing and got:

EBUILD_PHASE: clean
EBUILD_PHASE: cleanrm
EBUILD_PHASE: compile
EBUILD_PHASE: configure
EBUILD_PHASE: install
EBUILD_PHASE: instprep
EBUILD_PHASE: None
EBUILD_PHASE: package
EBUILD_PHASE: postinst
EBUILD_PHASE: postrm
EBUILD_PHASE: preinst
EBUILD_PHASE: prepare
EBUILD_PHASE: prerm
EBUILD_PHASE: setup
EBUILD_PHASE: test
EBUILD_PHASE: unpack

We don't even see a phase named "depend" there. Is that to be expeced?
From those phases, these are the ones that are neither "depend" or in _phase_func_map.keys():

clean
cleanrm
instprep
None
package

Note that we are also see an invocation where EBUILD_PHASE is None. Is that to be expected? I also see that the current approach leaves /tmp/portage-ebuild-extra-source-* files behind, created for the phases mentioned above (clean, cleanrm, instprep, None, package). Which would be in line with what we are seeing. I wonder if we shouldn't simply cleanup the file in the finally block at the end of the function, instead via EbuildMetadataPhase (That doesn't work in case returnpid or returnproc is active).

Copy link
Member Author

@Flowdalic Flowdalic Jan 11, 2025

Choose a reason for hiding this comment

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

A simple solution would be to only unexport the PMS variables if we are spawning in an EBUILD_PHASE in _phase_func_map.keys(), because then we can place the PORTAGE_EBUILD_EXTRA_SOURCE file inT. I've pushed this now.

The drawback would be, that the PMS variables are still exported when the ebuild is invoked without the intention to call a specific ebuild phase function. But it makes the implementation of the feature much cleaner, as we can bind the life cycle of the PORTAGE_EBUILD_EXTRA_SOURCE to T's life cycle.

If we also want to unexport the PMS vars in the clean, cleanrm, instprep, None, and package phases, then we need to come up with a good scheme to clean up the PORTAGE_EBUILD_EXTRA_SOURCE file at the end of those phases.

Copy link
Member

Choose a reason for hiding this comment

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

You will only observe the depend phase when there is missing metadata cache for a particular ebuild. Once the cache is populated, you won't observe it again until there is an ebuild or eclass modification.

Copy link
Member

Choose a reason for hiding this comment

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

If we also want to unexport the PMS vars in the clean, cleanrm, instprep, None, and package phases, then we need to come up with a good scheme to clean up the PORTAGE_EBUILD_EXTRA_SOURCE file at the end of those phases.

The problem is that we need to unexport them for all phases in order to prevent E2BIG errors.

bin/isolated-functions.sh Outdated Show resolved Hide resolved
@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch from 4f097cd to 1c4b427 Compare January 10, 2025 07:45
@Flowdalic
Copy link
Member Author

Now is a good time for others to test this patch and the feature. Set FEATURES="${FEATURES} -export-pms-vars" in make.conf to enable the behavior in all existing EAPIs. Make sure to use sys-apps/portage-9999, since sys-apps/portage was the only ebuild I have encountered so far that is affected by this change, and the -9999 ebuild of portage takes it into consideration.

@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch 4 times, most recently from b60b5dd to b2e47dc Compare January 11, 2025 10:07
Instead of passing PMS variables as part of the process environment,
we pass them via a file that is later sourced by the ebuild's
bash.

Since, for example, A is usually the greatest contributor to the
process environment, removing it from the process environment
significantly avoids running into MAX_ARG_STRLEN when spawning a new
child process.

This means that A and other PMS variables are no longer exported in
the ebuild and hence unavaiable to child processes. However, A is
mostly used as part of the default_src_unpack function and there A
does not need to be exported.

This started as a change that only unexported A, but was later
extended to most PMS variables as suggested by ulm in
https://bugs.gentoo.org/721088#c23.

Thanks to Zac Medico for helpful input on this change, and to Eli
Schwartz for suggesting that (bash) helpers should simply source the
environment file introduced by this change.

Closes: https://bugs.gentoo.org/721088
Signed-off-by: Florian Schmaus <[email protected]>
@Flowdalic Flowdalic force-pushed the pass-env-via-file-dont-export-pms-vars branch from b2e47dc to 9c7ea51 Compare January 11, 2025 10:08
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.

3 participants