Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 662586 - estack.eclass: QA Notice: Global shell options changed and were not restored while calling 'src_prepare'
Summary: estack.eclass: QA Notice: Global shell options changed and were not restored ...
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Current packages (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Gentoo's Team for Core System packages
URL:
Whiteboard:
Keywords:
: 638152 670318 670398 670404 670414 670426 670432 670442 670532 670544 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-08-01 16:19 UTC by Francesco Turco
Modified: 2021-01-16 15:48 UTC (History)
7 users (show)

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


Attachments
emerge-info.txt (emerge-info.txt,5.83 KB, text/plain)
2018-08-01 16:19 UTC, Francesco Turco
Details
build.log.xz (build.log.xz,143.79 KB, application/octet-stream)
2018-08-01 16:19 UTC, Francesco Turco
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Francesco Turco 2018-08-01 16:19:09 UTC
Created attachment 541948 [details]
emerge-info.txt

Portage reports the following QA issue:

> * QA Notice: Global shell options changed and were not restored while calling 'src_prepare'
Comment 1 Francesco Turco 2018-08-01 16:19:50 UTC
Created attachment 541950 [details]
build.log.xz
Comment 2 Sergei Trofimovich (RETIRED) gentoo-dev 2018-08-02 06:31:12 UTC
portage's QA warning should be improved to perform diff as well. emerge --debug shows it's a mismatch in:
  -expand_aliases         on
  +expand_aliases         off
Comment 3 Sergei Trofimovich (RETIRED) gentoo-dev 2018-08-02 07:33:08 UTC
Seems to be triggered by epatch() call on a directory in toolchain.eclass:src_prepare():

  EPATCH_MULTI_MSG="Applying Gentoo patches ..." \
    epatch "${WORKDIR}"/patch
Comment 4 Sergei Trofimovich (RETIRED) gentoo-dev 2018-08-02 19:11:08 UTC
Does not seem to be gcc-specific problem (it's trigger

$ cat sys-devel/bug/bug-0.ebuild
  # Copyright 1999-2018 Gentoo Foundation
  # Distributed under the terms of the GNU General Public License v2
  
  EAPI="6"
  
  inherit estack
  
  RDEPEND=""
  DEPEND=""
  SLOT="0"
  
  src_unpack() {
        mkdir "${S}"
        eshopts_push -o noglob
        eshopts_pop
  }

$ ebuild sys-devel/bug/bug-0.ebuild clean unpack
  >>> Unpacking source...
   * QA Notice: Global shell options changed and were not restored while calling 'src_unpack'

Reassigning to base-system@ as estack.eclass maintainer.
Comment 5 Sergei Trofimovich (RETIRED) gentoo-dev 2018-08-02 19:21:50 UTC
I think 'expand_aliases' is set in portage's helpers via bin/isolated-functions.sh:
    shopt -s expand_aliases
and does not get unset. I expect some portage like einfo/elog helper to set it at unexpected time.
Comment 6 Larry the Git Cow gentoo-dev 2018-08-08 08:07:18 UTC
The bug has been referenced in the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=e910f4f790f8da6d545f18cb1bc65abb3b60c81d

commit e910f4f790f8da6d545f18cb1bc65abb3b60c81d
Author:     Sergei Trofimovich <slyfox@gentoo.org>
AuthorDate: 2018-08-08 08:07:00 +0000
Commit:     Sergei Trofimovich <slyfox@gentoo.org>
CommitDate: 2018-08-08 08:07:10 +0000

    sys-devel/gcc: use 8.2.0 patchset for 8.2.0 release
    
    No change to gcc source tree.
    
    As EPATCH_EXCLUDE is not used anymore this change
    happens to workaround spurious QA warning in bug #662586.
    Bug: https://bugs.gentoo.org/662586
    Package-Manager: Portage-2.3.44, Repoman-2.3.10

 sys-devel/gcc/Manifest         |  1 +
 sys-devel/gcc/gcc-8.2.0.ebuild | 10 +---------
 2 files changed, 2 insertions(+), 9 deletions(-)
Comment 7 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2018-08-08 18:20:27 UTC
(In reply to Sergei Trofimovich from comment #5)
> I think 'expand_aliases' is set in portage's helpers via
> bin/isolated-functions.sh:
>     shopt -s expand_aliases
> and does not get unset. I expect some portage like einfo/elog helper to set
> it at unexpected time.

Simpler reproducer:

src_unpack() {
	local save=$(shopt -p -o)
	shopt -p > 1
	set -o noglob
	shopt -p > 2
	diff -dup 1 2
	eval "${save}"
	shopt -p > 3
	diff -dup 1 3
}

The eval call is causing the change in expand_aliases.  Looks like it's either a bug in bash or a feature.
Comment 8 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2018-08-08 18:23:11 UTC
By the way, the QA warning is not spurious.  The only spurious thing here is your arrogance which makes you commit broken stuff because you're confident it's not your code being broken.

You can read [1] as how to mangle options correctly, without undesirable side effects.

[1]:https://blogs.gentoo.org/mgorny/2016/01/25/mangling-shell-options-in-ebuilds/
Comment 9 Zac Medico gentoo-dev 2018-08-08 21:16:14 UTC
If it helps, I suppose we could eliminate the expand_aliases usage from portage. Currently portage only has two alias, save_IFS and restore_IFS, and they could easily be inlined since they are only used in one place.
Comment 10 Sergei Trofimovich (RETIRED) gentoo-dev 2018-08-08 21:25:06 UTC
(In reply to Michał Górny from comment #8)
> By the way, the QA warning is not spurious.

Totally could be.

> The only spurious thing here is
> your arrogance which makes you commit broken stuff because you're confident
> it's not your code being broken.

I suggest to continue in bug #663192 on this topic as it does not belong here.
Comment 11 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2018-08-10 07:03:35 UTC
I see that you really fail to notice what you've done.  You've left a *permanent false statement* about the QA warning being spurious in the git log where it was completely unnecessary and which does not provide any way to correct that statement.
Comment 12 Sergei Trofimovich (RETIRED) gentoo-dev 2018-08-10 22:07:59 UTC
(In reply to Michał Górny from comment #11)
> I see that you really fail to notice what you've done.

Yes. I can't see a point if it's not stated directly and clearly.

> You've left a
> *permanent false statement* about the QA warning being spurious 

I admit it was not a correct statement. People make mistakes and I do a _lot_ of them. If it's any consolation, commit message has a bug link. Here the bug should be explained clearer in comments and/or have an URL field to point to an authoritative doc (say, wiki) explaining the subtleties.

I have no 100% confidence in anything related to software and don't have that expectation from anyone else. All my statements should be taken with a grain of salt.

> in the git log where it was completely unnecessary

You did not say _why_ in your opinion commit wording:

1. is technically problematic: does it confuse you when you read it? Can it trick someone into applying incorrect fix to other packages? (Something else?)

2. is personally problematic to the point that it required an insult: did it attack you? If it was it unintentional. I don't enjoy being attacked myself (and was not able to think clearly for a day after all this)
   

> and which does not provide any way to correct that statement.

I don't understand this part. Is it about git history being immutable or something completely different? Does not bug link do?

If "why" part warrants the change I can do a git revert/git apply with amended message. Please provide your wording for the change for this case so I can evaluate the difference as sys-devel/gcc maintainer.
Comment 13 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2018-08-10 23:12:50 UTC
First of all, I'm sorry, I've misread your commit message.  After reading it a few times now, it finally got to me that you've made the warning disappear and not caused it.  So yes, the commit message is confusing a lot.

My point was that somebody having the same (or very similar issue) could -- upon reading this commit message -- wrongly assume the warning is spurious and ignore it.

And yes, git history is immutable.  That's why it is important to try to have good commit messages.
Comment 14 Sergei Trofimovich (RETIRED) gentoo-dev 2018-08-10 23:58:16 UTC
Got it. Thanks!
Comment 15 Larry the Git Cow gentoo-dev 2018-08-18 23:28:13 UTC
The bug has been referenced in the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=b21e386a064e1ed49981d9527f968ddc5e1eeb86

commit b21e386a064e1ed49981d9527f968ddc5e1eeb86
Author:     Sergei Trofimovich <slyfox@gentoo.org>
AuthorDate: 2018-08-10 23:46:23 +0000
Commit:     Sergei Trofimovich <slyfox@gentoo.org>
CommitDate: 2018-08-18 23:28:04 +0000

    epatch.eclass: drop 'estack.eclass' usage
    
    Avoid use of eshopts_push / eshopts_pop functions
    as they don't preserve expand_aliases shell option
    and get detected by QA warning as:
      * QA Notice: Global shell options changed and were
      not restored while calling 'src_prepare'
    
    Ssee bug #662586 for details.
    
    Tested as:
        $ EPATCH_USER_EXCLUDE="*" ebuild gcc-8.2.0.ebuild clean prepare
    
    Bug: https://bugs.gentoo.org/662586
    Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>

 eclass/epatch.eclass | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
Comment 16 Sergei Trofimovich (RETIRED) gentoo-dev 2018-09-02 00:25:04 UTC
*** Bug 638152 has been marked as a duplicate of this bug. ***
Comment 17 Ulrich Müller gentoo-dev 2018-11-07 08:29:36 UTC
*** Bug 670318 has been marked as a duplicate of this bug. ***
Comment 18 Ulrich Müller gentoo-dev 2018-11-07 08:29:39 UTC
*** Bug 670398 has been marked as a duplicate of this bug. ***
Comment 19 Ulrich Müller gentoo-dev 2018-11-07 08:29:41 UTC
*** Bug 670404 has been marked as a duplicate of this bug. ***
Comment 20 Ulrich Müller gentoo-dev 2018-11-07 08:29:43 UTC
*** Bug 670414 has been marked as a duplicate of this bug. ***
Comment 21 Ulrich Müller gentoo-dev 2018-11-07 08:29:45 UTC
*** Bug 670426 has been marked as a duplicate of this bug. ***
Comment 22 Ulrich Müller gentoo-dev 2018-11-07 08:29:47 UTC
*** Bug 670432 has been marked as a duplicate of this bug. ***
Comment 23 Ulrich Müller gentoo-dev 2018-11-07 08:29:50 UTC
*** Bug 670442 has been marked as a duplicate of this bug. ***
Comment 24 Ulrich Müller gentoo-dev 2018-11-07 08:29:52 UTC
*** Bug 670532 has been marked as a duplicate of this bug. ***
Comment 25 Ulrich Müller gentoo-dev 2018-11-07 08:29:54 UTC
*** Bug 670544 has been marked as a duplicate of this bug. ***
Comment 26 Ulrich Müller gentoo-dev 2019-11-24 12:48:29 UTC
Reported as Bash bug:
https://lists.gnu.org/archive/html/bug-bash/2019-11/msg00092.html
Comment 27 Larry the Git Cow gentoo-dev 2019-11-26 19:23:57 UTC
The bug has been closed via the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=d2cb9490dbee48a32f196d1aa80d7356a99d9fd8

commit d2cb9490dbee48a32f196d1aa80d7356a99d9fd8
Author:     Ulrich Müller <ulm@gentoo.org>
AuthorDate: 2019-11-23 10:18:18 +0000
Commit:     Ulrich Müller <ulm@gentoo.org>
CommitDate: 2019-11-26 19:23:19 +0000

    estack.eclass: Properly restore shopt options.
    
    Calling "eshopts_push; eshopts_pop" makes Portage report a QA issue:
    * QA Notice: Global shell options changed and were not restored while calling 'src_prepare'
    
    This is caused by some side effect in bash, by which disabling
    the "posix" option (even if it was already disabled before) in a
    non-interactive shell also disables the "expand_aliases" option.
    Work around the problem by always saving and restoring both "set -o"
    and "shopt" option sets.
    
    Also fix "estack_push -s" which should not execute shopt when called
    without further parameters.
    
    Closes: https://bugs.gentoo.org/662586
    Signed-off-by: Ulrich Müller <ulm@gentoo.org>

 eclass/estack.eclass | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
Comment 28 Ulrich Müller gentoo-dev 2021-01-16 15:48:23 UTC
(In reply to Ulrich Müller from comment #26)
> Reported as Bash bug:
> https://lists.gnu.org/archive/html/bug-bash/2019-11/msg00092.html

This is fixed in bash-5.1.