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

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

Closed
wants to merge 1 commit into from

Conversation

mingwandroid
Copy link
Contributor

@mingwandroid mingwandroid commented Jan 10, 2017

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.

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
Copy link
Contributor Author

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

@mingwandroid
Copy link
Contributor Author

mingwandroid commented Jan 10, 2017

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

@kalefranz
Copy link
Contributor

kalefranz commented Jan 10, 2017 via email

@mingwandroid
Copy link
Contributor Author

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

@mingwandroid
Copy link
Contributor Author

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

sed --help | grep \\-E?

@kalefranz
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@kalefranz
Copy link
Contributor

closing in lieu of #4257

@kalefranz kalefranz closed this Jan 12, 2017
@mbargull mbargull added the cli pertains to the CLI interface label Mar 13, 2018
@github-actions
Copy link

github-actions bot commented Sep 5, 2021

Hi there, thank you for your contribution to Conda!

This pull request has been automatically locked since it has not had recent activity after it was closed.

Please open a new issue or pull request if needed.

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Sep 5, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cli pertains to the CLI interface locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants