Prefix is self-contained, tools from the host is not needed. It is not desired that host pkg-config leaks host libraries into ebuild environment. This patch fixes it. Reproducible: Always
Created attachment 437616 [details, diff] portage-2.3.0-prefix-path-only.patch
While I do see this could help to identify erroneous usage of host tools, Prefix eventually is not self-contained on each platform and each setup, so I don't think omitting the host path in general is a good idea. And "prefix-chaining" (bug#509136) comes into mind as well.
Hi Michael, (In reply to Michael Haubenwallner from comment #2) > While I do see this could help to identify erroneous usage of host tools, > Prefix eventually is not self-contained on each platform and each setup, so > I don't think omitting the host path in general is a good idea. > And "prefix-chaining" (bug#509136) comes into mind as well. The prefix branch of portage has EXTRA_PATH and DEFAULT_PATH that can be set in the profiles to point to host tools. This proposed patch removes host tools by default, and shift the duty from portage to profiles.
(In reply to Benda Xu from comment #3) > (In reply to Michael Haubenwallner from comment #2) > > While I do see this could help to identify erroneous usage of host tools, > > Prefix eventually is not self-contained on each platform and each setup, so > > I don't think omitting the host path in general is a good idea. > > And "prefix-chaining" (bug#509136) comes into mind as well. > > The prefix branch of portage has EXTRA_PATH and DEFAULT_PATH that can be set > in the profiles to point to host tools. > > This proposed patch removes host tools by default, and shift the duty from > portage to profiles. Ok, sounds reasonable, thanks!
Ping portage team.
(In reply to Benda Xu from comment #1) > Created attachment 437616 [details, diff] [details, diff] > portage-2.3.0-prefix-path-only.patch Looks good. (In reply to Benda Xu from comment #3) > The prefix branch of portage has EXTRA_PATH and DEFAULT_PATH that can be set > in the profiles to point to host tools. Why introduce EXTRA_PATH and DEFAULT_PATH be needed, when we already have PREROOTPATH and ROOTPATH variables? Why should the prefix branch diverge from the master branch here?
(In reply to Zac Medico from comment #6) > Why introduce EXTRA_PATH and DEFAULT_PATH be needed, when we already have > PREROOTPATH and ROOTPATH variables? Why should the prefix branch diverge > from the master branch here? @Fabian, do you have an idea about this?
DEFAULT_PATH and EXTRA_PATH used to come from make.globals. (In Prefix they still do.) ROOTPATH and PREROOTPATH (appears to me to) come from /etc/profile.env, which means it is constructed from files in /etc/env.d. Since bootstrapping portage needs the path respected, one would get a scenario where the env isn't setup because one needs portage (env-update) to set it. So, with the current state, it seems things have changed, path setup is moved down into doebuild.sh, and it prefixes the paths already, which should take care of DEFAULT_PATH. The EXTRA_PATH is there, because some platforms need some special dirs added in order to find tools. So during bootstrap, one might find there /usr/sfw/.../bin, /usr/gnu/bin, /opt/local/gccXX/bin/, /Library/Developer/CommandLineTools/usr/bin and more. Post bootstrap, it's unlikely we need these. So because Portage disregards PATH from env (= good thing IMO) we need a trick to make it pick up the odd paths from above during bootstrap. DEFAULT_PATH can go.
That all said, means that if Benda's patch gets through we need DEFAULT_PATH again, because he'd remove the host paths we DO need in Prefix.
The conclusion is that we can land this patch on portage, and use DEFAULT_PATH and EXTRA_PATH in the prefix branch. @Zac, could you please help incorporate it upstream? Thanks.
Created attachment 449600 [details, diff] portage-2.3.0-prefix-path.patch I revised to patch. If EPREFIX is set in the environment and is different from portage.const.EPREFIX (when doing cross-eprefix), PATH of EPREFIX should be used first. Many tools, particularly eselect and gcc-config (in the main tree), hardcodes EPREFIX. Therefore calling eselect from env EPREFIX is necessary during cross-eprefix. Please tell me your opinion about this.
(In reply to Benda Xu from comment #11) > + # settings["EPREFIX"] could be overridden during cross-eprefix > + if portage.const.EPREFIX != settings["EPREFIX"]: > + prefixes.append(portage.const.EPREFIX) I think this would be safer, and more compatible with the previous behavior where prefixes.append("/") was unconditional: if not prefixes or portage.const.EPREFIX != settings["EPREFIX"]: prefixes.append(portage.const.EPREFIX)
(In reply to Zac Medico from comment #13) > (In reply to Benda Xu from comment #11) > > + # settings["EPREFIX"] could be overridden during cross-eprefix > > + if portage.const.EPREFIX != settings["EPREFIX"]: > > + prefixes.append(portage.const.EPREFIX) > > I think this would be safer, and more compatible with the previous behavior > where prefixes.append("/") was unconditional: > > if not prefixes or portage.const.EPREFIX != settings["EPREFIX"]: > prefixes.append(portage.const.EPREFIX) Hello Zac, The rationale here is to fallback to portage.const.EPREFIX when a tool is not found in settings["EPREFIX"]. Therefore, I think the original form is more proper. With the previous behavior using portage.const.EPREFIX unconditionally, I think the original form is more compatible. Thanks, Benda
Ping portage Team. Would you please show some love to this bug?
It looks fine to me but there where unit test failures. I've sent this pull request: https://github.com/gentoo/portage/pull/385 The unit test results will be available here: https://travis-ci.org/gentoo/portage/builds/466355303
(In reply to Zac Medico from comment #16) > It looks fine to me but there where unit test failures. I've sent this pull > request: > > https://github.com/gentoo/portage/pull/385 > > The unit test results will be available here: > > https://travis-ci.org/gentoo/portage/builds/466355303 The test has set a non-existent EPREFIX "/tmp/tmpXXXXXXX", both settings["EPREFIX"] and portage.const.EPREFIX. I think in this case, portage should fail to find essental commands, like `chown`, `mkdir`, etc. What is the rationale to set EPREFIX=/tmp/tmpXXXXXXXX in the test? Benda
The unit tests fail due to commands that are no longer in the default PATH setting: > /home/travis/build/gentoo/portage/build/bin/isolated-functions.sh: line 83: basename: command not found > /home/travis/build/gentoo/portage/build/bin/isolated-functions.sh: line 436: uname: command not found > /home/travis/build/gentoo/portage/build/bin/phase-functions.sh: line 1109: chmod: command not found > /home/travis/build/gentoo/portage/build/bin/isolated-functions.sh: line 436: install: command not found
(In reply to Benda Xu from comment #17) > What is the rationale to set EPREFIX=/tmp/tmpXXXXXXXX in the test? This allows it to test complete invocations for commands like emerge. Maybe we should update ResolverPlayground to create symlinks to required programs in ${EPREFIX}/bin.
The test cases are good ones! It caught an implicit bug for cross_root. The refreshed pull request at https://github.com/gentoo/portage/pull/387 addresses this issue. Please review.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/portage.git/commit/?id=1b5edbb5ec70f1ae95b8f538a02037bb9a1ded9f commit 1b5edbb5ec70f1ae95b8f538a02037bb9a1ded9f Author: Benda Xu <heroxbd@gentoo.org> AuthorDate: 2018-12-13 08:53:16 +0000 Commit: Zac Medico <zmedico@gentoo.org> CommitDate: 2018-12-13 20:34:02 +0000 _doebuild_path: do not use host PATH by default and prepend EPREFIX PATH. EPREFIX could be overridden in cross-eprefix, in that case tools inside EPREFIX should be prioritized. Link necessary binary tools to the test environment. Bug: https://bugs.gentoo.org/585986 Closes: https://github.com/gentoo/portage/pull/387 Signed-off-by: Zac Medico <zmedico@gentoo.org> lib/portage/package/ebuild/doebuild.py | 9 ++++--- lib/portage/tests/resolver/ResolverPlayground.py | 34 ++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-)
(In reply to Benda Xu from comment #10) > The conclusion is that we can land this patch on portage, and use > DEFAULT_PATH and EXTRA_PATH in the prefix branch. > > @Zac, could you please help incorporate it upstream? Thanks. Will DEFAULT_PATH and EXTRA_PATH remain in the prefix branch? If that's the case, should we merge support for them into the master branch? If so, would you like to rebase patch for DEFAULT_PATH and EXTRA_PATH on master? https://gitweb.gentoo.org/proj/portage.git/commit/?id=693812e7d867826da51a677f43f5b8566ca5d393
The difference here is that Prefix considers itself the replacement of /usr/local (sort of). So DEFAULT_PATH and EXTRA_PATH currently don't have /usr/local/{s,}bin variants in them. I don't see adding /usr/local to DEFAULT_PATH as a big problem, EXTRA_PATH we would still need, and that one would just lack /usr/local (we control it). So, I could see possibility here to: - drop DEFAULT_PATH, use the path generation as introduced by recent commit - make addition to add EXTRA_PATH if set (or if in Prefix)
(In reply to Fabian Groffen from comment #23) > The difference here is that Prefix considers itself the replacement of > /usr/local (sort of). So DEFAULT_PATH and EXTRA_PATH currently don't have > /usr/local/{s,}bin variants in them. > > I don't see adding /usr/local to DEFAULT_PATH as a big problem, EXTRA_PATH > we would still need, and that one would just lack /usr/local (we control it). > > So, I could see possibility here to: > - drop DEFAULT_PATH, use the path generation as introduced by recent commit > - make addition to add EXTRA_PATH if set (or if in Prefix) Thanks Fabian, I agree with this option to move forward. Benda
Merged and adapted in prefix branch as 6d68e3cef901f3322a80525b472fb04d1304de5a
Fixed in portage-2.3.62.
