-
Notifications
You must be signed in to change notification settings - Fork 153
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
base: master
Are you sure you want to change the base?
update regex to support BSD sed (for macOS devs) and GNU sed #293
Conversation
- Fix trimming of matching quote (no star after \1 reference) - Only trim a single quote (turn any-match into optional match)
There was a problem hiding this 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.
lib/shared-functions.sh
Outdated
@@ -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/" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:-}"
)
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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