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

fix: make the sed expression work for non-GNU sed #65

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

dochang
Copy link
Contributor

@dochang dochang commented Apr 14, 2023

Use ERE instead, because GitHub Actions doesn't use GNU sed on macOS by default.

@jthegedus
Copy link
Collaborator

From memory we don't use sed -E flag in the asdf core because it is also not very portable. We use regular sed without -E in the core and our tests mostly fine. What's the case that is failing that requires this?

@dochang
Copy link
Contributor Author

dochang commented Apr 18, 2023

Thanks for your reply.

Last week I submitted a patch makes latest-stable support version numbers without "v" prefix. But I found that my plugin's test failed on macos on Github Actions.

The reason

The reason is that Github Actions uses BSD sed by default, which uses a different BRE syntax from GNU sed.

Check this commit 6c2416e and its action run 4734069256, latest-stable fails on macos when sed 's|.*/tag/v\?||' is used.

According to the manual of GNU sed, \? is an extensions:

\?

As *, but only matches zero or one. It is a GNU extension.

Instead, macos uses ?, according to this manpage:

An atom followed by '?' matches a sequence of 0 or 1 matches of the atom.

Possible solutions

We have two choices:

  1. Replace \? with \{0,1\}

See this commit 83448d7 and its action run 4734486470

  1. Use sed -E (ERE)

According to this manpage, macos has supported ERE since 2005.

See this commit ef8ec79 and its action run 4734836943

Conclusion

Both solutions are ok. I prefer ERE. So I send this PR.

@jthegedus
Copy link
Collaborator

jthegedus commented Apr 18, 2023

I would prefer a solution without -E. Although macOS supports it, users run asdf on Alpine amongst other linux distros which may not have -E support in the way we expect. We don't currently test for all OS permutations of our users. Just the fact we don't use -E in the core means I don't want to introduce it in the plugin-template.

@dochang
Copy link
Contributor Author

dochang commented Apr 19, 2023

That makes sense. Let's go with \{0,1\}.

Use ERE instead, because GitHub Actions doesn't use GNU sed on macOS by default.
@dochang
Copy link
Contributor Author

dochang commented Apr 19, 2023

@jthegedus A new commit has been pushed into this PR.

Copy link
Collaborator

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

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.

None yet

2 participants