{de,}activate: Remove BSD-ism from sed calls #4249

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@mingwandroid
Contributor
mingwandroid commented Jan 10, 2017 edited

The -E flag only works on macOS (and some *BSD I guess). In GNU sed, the flag is
-r. These flags turn on 'extended regular expressions'.

Extended regular expressions were only needed in one of the three invocations (the
other case where initially it looked like it serves a purpose removed the : only
to put it back again).

The case that did make use of extended regular expressions is rewritten to use sed
'chaining' instead. I could've added code to determine which sed is being used but
macOS sed does not provide a --version flag and even if it did, calling it twice
would add the overhead of another process invocation.

@mingwandroid mingwandroid {de,}activate: Remove BSD-ism from sed calls
The `-E` flag only works on macOS (and some *BSD I guess). In GNU sed, the flag is
`-r`. These flags turn on 'extended regular expressions'.

Extended regular expressions were only needed in one of the three invocations (the
other case where initially it looked like it serves a purpose removed the `:` only
to put it back again.

The case that did make use of extended regular expressions is rewritten to use sed
'chaining' instead. I could've added code to determine which sed is being used but
macOS sed does not provide a `--version` flag and even if it did, calling it twice
would add the overhead of another process invocation.
96d1544
@mingwandroid
Contributor

4.2.14 doesn't work on Linux (was it even tested?!). Please merge this to 4.3.x too.

@mingwandroid
Contributor
mingwandroid commented Jan 10, 2017 edited

I cannot use python setup.py install to install my fix for this. Issue filed at #4251

@kalefranz
Member
@mingwandroid
Contributor

If this was tested, can you please fix your tests?

@mingwandroid
Contributor

I actually questioned the -E flag, but google convinced me it was safe across distros.

sed --help | grep \\-E?

@kalefranz
Member

your tests fail here. And I'm fixing this, but...

https://www.gnu.org/software/sed/manual/sed.txt

'-E'
'-r'
'--regexp-extended'
Use extended regular expressions rather than basic regular
expressions. Extended regexps are those that 'egrep' accepts; they
can be clearer because they usually have fewer backslashes.
Historically this was a GNU extension, but the '-E' extension has
since been added to the POSIX standard
(http://austingroupbugs.net/view.php?id=528), so use '-E' for
portability. GNU sed has accepted '-E' as an undocumented option
for years, and *BSD seds have accepted '-E' for years as well, but
scripts that use '-E' might not port to other older systems. *Note
Extended regular expressions: ERE syntax.

@mingwandroid
Contributor

That conda isn't corrupting PATH isn't something I have any tests for.

Unfortunately 'real' Linuxes all seem to use sed <= 4.2.2. Documentation on GNU's website isn't a substitute for actually checking.

@mingwandroid
Contributor

FYI, there has been exactly one release of GNU sed with this feature, sed 4.3, release on 4th Jan 2017.

@kalefranz
Member

closing in lieu of #4257

@kalefranz kalefranz closed this Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment