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

update regex to support BSD sed (for macOS devs) and GNU sed #293

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hpccpatest
Copy link

@hpccpatest hpccpatest commented Sep 30, 2024

Description

The sed parsing present in lib/shared_function.sh does not work correctly on macOS. I think it is because macOS by default uses BSD sed instead of GNU sed (for instance using \1 within the regex expression works on GNU sed but not on BSD sed, however I need to find documentation confirming this). Updated the regex so it works correctly for both BSD and GNU sed (tested it locally and worked fine on both).

One possible alternative to this PR could be to add a note in the guide for macOS users so they are aware of the issue and can workaround it by installing GNU sed and using that in the script, however I think this PR makes it easier and reduces the onus on new developers.

Apart from fixing #287, this PR also stops the regex from matching/including ending quotes as part of the captured group if they are present.

Related issues / Pull Requests

Fixes #287

Contributor Agreement

hpccpatest referenced this pull request Sep 30, 2024
- Fix trimming of matching quote (no star after \1 reference)
- Only trim a single quote (turn any-match into optional match)
Copy link
Member

@das7pad das7pad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this! Unquoting really is tricky 🙃

I'll check with the team on how to resolve this.

@@ -180,11 +180,11 @@ function check_sharelatex_env_vars() {
function read_variable() {
local name=$1
grep -E "^$name=" "$TOOLKIT_ROOT/config/variables.env" \
| sed -r "s/^$name=([\"']?)(.+)\1\$/\2/"
| sed -r "s/^$name=([\"']?)([^\"']+)[\"']?$/\2/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not trim in case there are quotes inside the value, e.g. foo='Instance managed by "Department"'.

before:

$ name=foo; cat | sed -r "s/^$name=([\"']?)(.+)\1\$/\2/"
foo='Instance managed by "Department"'

// output
Instance managed by "Department"

after:

$ name=foo; cat | sed -r "s/^$name=([\"']?)([^\"']+)[\"']?$/\2/"
foo='Instance managed by "Department"'

// output
foo='Instance managed by "Department"'

Using a non-greedy matching group instead of the "non-quote"-group in the middle does not work either:

$ name=foo; cat | sed -r "s/^$name=([\"']?)(.+?)[\"']?$/\2/"
foo='Instance managed by "Department"'

// output
Instance managed by "Department"'
                                ^-- unmatched quote

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using bash to do the parsing in a subshell?

function read_variable() {
  local name=$1
  (
    source "$TOOLKIT_ROOT/config/variables.env"
    echo "${!name:-}"
  )
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@das7pad good point, I didn't think values for environment variables would have quotes in most cases, but I agree it makes sense to support that as well. Thanks for pointing that out.

@briangough thank you for looking into this. I think your solution is much simpler and cleaner and works well for the above mentioned edge case as well. I'll update the PR with it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@das7pad just wanted to check-in, if we can merge this PR or is there anything else I need to do prior to that? Thanks.

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

Successfully merging this pull request may close these issues.

bin/up / Quick start guide fails due to incorrect MONGO_VERSION parsing
3 participants