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: sed invocations to work out of the box on macOS #615

Merged
merged 6 commits into from
Jun 3, 2023

Conversation

TomasHubelbauer
Copy link
Contributor

These fixes make the sed invocations run on macOS but I am not sure if they will break the GNU sed invocations in return… I am hoping the GitHub Actions workflow will pass and config this is now portable between macOS and Ubuntu (what GitHub Actions is using).

These fixes make the sed invocations run on macOS but I am not sure if they will break the GNU sed invocations in return…
@TomasHubelbauer
Copy link
Contributor Author

From the GitHub Actions workflow:

+ ./bin/build-contributors.sh
sed: can't read : No such file or directory
sed: can't read : No such file or directory

This is what I was worried about, my changes made this portable for BSD sed but in the process broke it for GNU sed. I will try to add OS detection and call sed differently for macOS.

The sed command line was difficult (impossible?) to be made portable so I just added macOS conditions.
@TomasHubelbauer
Copy link
Contributor Author

Alright I just added macOS specific branches to the shell scripts and I call sed differently depending on what the OS is the script is running under. Now the CI passes under Ubuntu so I don't think I broke the GNU sed invocations and it also works on macOS locally so BSD sed invocations work as well.

bin/build-changelog.sh Outdated Show resolved Hide resolved
bin/build-contributors.sh Outdated Show resolved Hide resolved
It seems that this will not work because I keep getting "sedi: command not found".
@harttle
Copy link
Owner

harttle commented May 29, 2023 via email

This seems to work on my local so let's see if it runs in the CI on GitHub.
Not sure why VS Code went with 4.
TomasHubelbauer added a commit to TomasHubelbauer/liquidjs that referenced this pull request May 29, 2023
…macOS as well

Once harttle#615 is merged, the build script should work on macOS just as well as it does on Ubuntu and with this GitHub Actions matrix we can keep ensuring that it continues to work as it should.
TomasHubelbauer added a commit to TomasHubelbauer/liquidjs that referenced this pull request May 29, 2023
…ld on macOS as well

Once harttle#615 is merged, the build script should work on macOS just as well as it does on Ubuntu and with this GitHub Actions matrix we can keep ensuring that it continues to work as it should.
bin/build-changelog.sh Outdated Show resolved Hide resolved
I am curious if Ubuntu will handle the newline…
@harttle harttle merged commit 87d4cc7 into harttle:master Jun 3, 2023
harttle added a commit that referenced this pull request Jun 3, 2023
@TomasHubelbauer TomasHubelbauer deleted the tom/sed-macos branch June 3, 2023 14:58
harttle pushed a commit that referenced this pull request Jun 3, 2023
…ld on macOS as well

Once #615 is merged, the build script should work on macOS just as well as it does on Ubuntu and with this GitHub Actions matrix we can keep ensuring that it continues to work as it should.
github-actions bot pushed a commit that referenced this pull request Jun 3, 2023
# [10.8.0](v10.7.1...v10.8.0) (2023-06-03)

### Bug Fixes

* proper error message for filter syntax error, [#610](#610) ([0480d33](0480d33))
* sed invocations to work out of the box on macOS ([#615](#615)) ([87d4cc7](87d4cc7))

### Features

* Add support for the Jekyll sample filter ([#612](#612)) ([ba8b842](ba8b842))
* introduce a matrix with latest Ubuntu and macOS to test the build on macOS as well ([82ba548](82ba548)), closes [#615](#615)
* precise line/col for tokenization Error, [#613](#613) ([e347e60](e347e60))
@github-actions
Copy link

github-actions bot commented Jun 3, 2023

🎉 This PR is included in version 10.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

TomasHubelbauer added a commit to TomasHubelbauer/liquidjs that referenced this pull request Jun 4, 2023
* fix: sed invocations to work out of the box on macOS

These fixes make the sed invocations run on macOS but I am not sure if they will break the GNU sed invocations in return…

* fix: add macOS branches to the shell scripts

The sed command line was difficult (impossible?) to be made portable so I just added macOS conditions.

* Prototype the solution using an alias

It seems that this will not work because I keep getting "sedi: command not found".

* fix: use a Bash function instead of an alias to run sed portably

This seems to work on my local so let's see if it runs in the CI on GitHub.

* fix: use 2 spaces like the original scripts did

Not sure why VS Code went with 4.

* fix: use sedi for the build-changelog `1i\` part as well

I am curious if Ubuntu will handle the newline…
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.

2 participants