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

Use POSIX.2 compatible sed -E for duplicate slash removal #4711

Merged
merged 5 commits into from
Jun 23, 2019

Conversation

trinitronx
Copy link
Contributor

@trinitronx trinitronx commented Jun 11, 2019

Fixes #4618

Changes proposed in this pull request:

  • Avoid platform detection required for sed -r call
  • Use POSIX.2 standard flag sed -E instead

@pkuczynski
Copy link
Member

@mpapis what you think?

@pkuczynski pkuczynski added this to the rvm-1.29.9 milestone Jun 11, 2019
@pkuczynski pkuczynski added the bug label Jun 11, 2019
scripts/functions/utility Show resolved Hide resolved
@mpapis
Copy link
Member

mpapis commented Jun 13, 2019

@trinitronx thanks for pointing this out - I was so bent on saving the code from breaking apart in the rare cases I lost the real issue from my eyes - calling to sed to do a shell operation, in 99.99% cases it's faster to run something in shell then to subprocess to other tool like sed or whatever

# x=a///////b
# echo $x
a///////b
# while [[ "$x" == *"//"* ]] ; do x="${x/\/\///}" ; done ; echo $x
a/b

@pkuczynski
Copy link
Member

@trinitronx can you apply @mpapis suggestion?

@pkuczynski pkuczynski changed the title Use POSIX.2 compatible sed -E for duplicate slash removal (fixes rvm/rvm#4618) Use POSIX.2 compatible sed -E for duplicate slash removal Jun 19, 2019
@pkuczynski
Copy link
Member

@trinitronx?

@trinitronx
Copy link
Contributor Author

@pkuczynski: Sure, I'll try to find some time to implement this.

@mpapis: Just to be clear, we're talking about sidestepping the issue using plain Bash substring manipulation instead of a call to sed, correct?

@trinitronx
Copy link
Contributor Author

trinitronx commented Jun 21, 2019

in 99.99% cases it's faster to run something in shell then to subprocess to other tool like sed or whatever

Looks like it's correct in this case:

$ export x="/Users//jcuzella/.rvm/gems/ruby-2.4.0@sprout-wrap/bin/:/Users//jcuzella/.rvm/gems/ruby-2.4.0@global/bin:/Users/jcuzella/.rvm/rubies/ruby-2.4.0/bin:/Users/jcuzella/.rvm/bin/:/usr/local/opt/openssl/bin:/usr/local/sbin:/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Applications/VMware Fusion.app/Contents/Public:/usr/local/MacGPG2/bin:/opt/X11/bin:/Applications/Wireshark.app/Contents/MacOS"

$ time g

real	0m0.008s
user	0m0.002s
sys	0m0.005s

$ export g="$x"

$ export x="/Users//jcuzella/.rvm/gems/ruby-2.4.0@sprout-wrap/bin/:/Users//jcuzella/.rvm/gems/ruby-2.4.0@global/bin:/Users/jcuzella/.rvm/rubies/ruby-2.4.0/bin:/Users/jcuzella/.rvm/bin/:/usr/local/opt/openssl/bin:/usr/local/sbin:/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Applications/VMware Fusion.app/Contents/Public:/usr/local/MacGPG2/bin:/opt/X11/bin:/Applications/Wireshark.app/Contents/MacOS"

$ time f

real	0m0.002s
user	0m0.002s
sys	0m0.000s

$ export f="$x"

$ type f
f is a function
f ()
{
    while [[ "$x" == *"//"* ]]; do
        x="${x/\/\///}";
    done;
    while [[ "$x" == *"/:"* ]]; do
        x="${x/\/:/:}";
    done
}

$ type g
g is a function
g ()
{
    x="$(\sed -E -e 's#/+#/#g' -e 's#/:#:#g' <<<$x)"
}

$ shasum -a 256 <(echo -n "$f") <(echo -n "$g")
662f46af8ec8510376fcf7c76a56d5ed76fe7c010782df6efc047cfee55fe7b8  /dev/fd/63
662f46af8ec8510376fcf7c76a56d5ed76fe7c010782df6efc047cfee55fe7b8  /dev/fd/62

@trinitronx
Copy link
Contributor Author

Implemented using @mpapis' example above

Pushed alternate solution in bda5875

@trinitronx
Copy link
Contributor Author

@mpapis, @pkuczynski : Merge conflicts fixed for CHANGELOG.md

mpapis
mpapis previously approved these changes Jun 23, 2019
@mpapis mpapis merged commit 8a3bf6a into rvm:master Jun 23, 2019
@mpapis
Copy link
Member

mpapis commented Jun 23, 2019

@trinitronx Thank you, pushed it one step further - dropped the unnecessary if. This said - there are no more path operations with subprocesses, if you notice other places that could benefit from this update feel free to open a PR - this is speeding up RVM.

@efojs
Copy link

efojs commented Jun 29, 2019

@trinitronx I'm on OS X El Capitan, upgraded RVM to 1.29.8 and also stuck with this problem (sed: illegal option -- r). Thank you a lot!

@PeterMozesMerl
Copy link

PeterMozesMerl commented Jul 9, 2019

I’m coming from #4711

Maybe the following will be nothing new to you. Let’s make it sure.

I experienced this issue for the first time only today even though I’ve been using the same RVM, Ruby, Rails, and macOS version for a long time. It was triggered only by a new Rails project.

The issue appears only if the Rails project has either a .ruby-version file or a ruby line in the Gemfile. As these two are amongst the few things I always delete before I add a new Rails project into the repository, my existing Rails project worked fine.

In case someone is looking for a solution now, delete these unless you rely on them. I don’t. Moreover, the only issues I’ve ever had with RVM were also related to this file and Gemfile line.

Usually, I use different Ruby versions on my development machines, and another version on the CI and the production servers. (I have the same Ruby version as well as the same OS on the CI and the production servers). On a specific machine, all my projects are running under the same Ruby version though.

I’m not sure why we have them. I would think it’s more likely that one needs to use a different Ruby version on another platform than that one needs to use a different Ruby version for another Ruby/Rails project.

Maybe they are needed for some commercial hosting service. I don’t know. But as soon as you deploy to your servers, the .ruby-version file is only noise in the VCS at best, and once in a while, a source of issues.

(I guess RVM doesn't create them, but Rails does. I never looked into it. I delete them for my mental peace.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to latest RVM breaks on OSX
5 participants