Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 585986 - portage should prepend environment EPREFIX PATH instead of appending host PATH
Summary: portage should prepend environment EPREFIX PATH instead of appending host PATH
Status: RESOLVED FIXED
Alias: None
Product: Portage Development
Classification: Unclassified
Component: Core (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Portage team
URL:
Whiteboard:
Keywords: InVCS
Depends on:
Blocks: 671498
  Show dependency tree
 
Reported: 2016-06-15 03:03 UTC by Benda Xu
Modified: 2019-04-10 04:37 UTC (History)
1 user (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
portage-2.3.0-prefix-path-only.patch (portage-2.3.0-prefix-path-only.patch,1.06 KB, patch)
2016-06-15 03:03 UTC, Benda Xu
Details | Diff
portage-2.3.0-prefix-path.patch (portage-2.3.0-prefix-path-only.patch,1.68 KB, patch)
2016-10-09 07:52 UTC, Benda Xu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benda Xu gentoo-dev 2016-06-15 03:03:09 UTC
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
Comment 1 Benda Xu gentoo-dev 2016-06-15 03:03:57 UTC
Created attachment 437616 [details, diff]
portage-2.3.0-prefix-path-only.patch
Comment 2 Michael Haubenwallner (RETIRED) gentoo-dev 2016-06-15 06:25:50 UTC
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.
Comment 3 Benda Xu gentoo-dev 2016-06-15 08:21:26 UTC
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.
Comment 4 Michael Haubenwallner (RETIRED) gentoo-dev 2016-06-15 08:35:59 UTC
(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!
Comment 5 Benda Xu gentoo-dev 2016-07-07 01:29:45 UTC
Ping portage team.
Comment 6 Zac Medico gentoo-dev 2016-07-07 05:51:12 UTC
(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?
Comment 7 Benda Xu gentoo-dev 2016-07-17 10:46:52 UTC
(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?
Comment 8 Fabian Groffen gentoo-dev 2016-07-18 08:35:23 UTC
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.
Comment 9 Fabian Groffen gentoo-dev 2016-07-18 08:36:53 UTC
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.
Comment 10 Benda Xu gentoo-dev 2016-10-08 13:45:27 UTC
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.
Comment 11 Benda Xu gentoo-dev 2016-10-09 07:52:38 UTC
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.
Comment 12 Benda Xu gentoo-dev 2017-01-07 15:08:55 UTC
Ping portage team.
Comment 13 Zac Medico gentoo-dev 2017-01-08 09:43:44 UTC
(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)
Comment 14 Benda Xu gentoo-dev 2017-12-11 08:43:15 UTC
(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
Comment 15 Benda Xu gentoo-dev 2018-12-11 04:44:19 UTC
Ping portage Team.  Would you please show some love to this bug?
Comment 16 Zac Medico gentoo-dev 2018-12-11 05:55:52 UTC
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
Comment 17 Benda Xu gentoo-dev 2018-12-11 11:59:27 UTC
(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
Comment 18 Zac Medico gentoo-dev 2018-12-11 20:25:58 UTC
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
Comment 19 Zac Medico gentoo-dev 2018-12-11 20:29:37 UTC
(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.
Comment 20 Benda Xu gentoo-dev 2018-12-13 13:42:18 UTC
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.
Comment 21 Larry the Git Cow gentoo-dev 2018-12-13 20:36:19 UTC
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(-)
Comment 22 Zac Medico gentoo-dev 2018-12-16 09:15:09 UTC
(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
Comment 23 Fabian Groffen gentoo-dev 2018-12-16 10:19:47 UTC
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)
Comment 24 Benda Xu gentoo-dev 2018-12-16 14:22:33 UTC
(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
Comment 25 Fabian Groffen gentoo-dev 2018-12-23 11:18:50 UTC
Merged and adapted in prefix branch as 6d68e3cef901f3322a80525b472fb04d1304de5a
Comment 26 Zac Medico gentoo-dev 2019-04-10 04:37:05 UTC
Fixed in portage-2.3.62.