Skip to content

Conversation

jenstroeger
Copy link
Contributor

@jenstroeger jenstroeger commented Sep 29, 2024

Fix to PR #853 (tagging @mabdollahpour-ol)

The Makefile goal assumed Brew only, so I added support for Ports and a check for gsed to avoid installing it twice. Also note that at least Ports aliases gsed as sed such that

SED="sed"
# Use gnu-sed (gsed) on mac.
if [[ "$(uname)" == "Darwin" ]]; then
SED="gsed"
fi

becomes irrelevant. I don’t know how Brew manages overriding macOS CLI tools with GNU tools.

Furthermore, I cleaned up indentation to be consistent with most (not all!) of the Makefile: every line of a rule indents by a single tab, then two spaces within scripts. That way it is easier to tell the difference between tab-indent (four or eight, depending on the editor) and space-indent (always two), and it makes managing block-indents easier & consistent.

Lastly, I think this line

echo "Unable to install Souffle. Please install it manually." && exit 0; \
should contain an exit 1 to communicate the failure correctly to the running shell.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 29, 2024
@jenstroeger jenstroeger force-pushed the fix-makefile-sed branch 2 times, most recently from 1a83d92 to 4dc7a86 Compare September 29, 2024 22:50
@mabdollahpour-ol
Copy link
Contributor

Thanks!

I don’t know how Brew manages overriding macOS CLI tools with GNU tools.

It doesn't anymore. It used to have --with-default-names that would override the apple stuff but now it lets the users to do the aliasing/overriding themselves. brew installs the gnu stuff with a g prefix. That's why there is gsed in the script.

The Makefile goal assumed Brew only

Not just this goal. We only use brew to install souffle as well. We should probably be consistent in using these tools.

@jenstroeger
Copy link
Contributor Author

jenstroeger commented Sep 29, 2024

Not just this goal. We only use brew to install souffle as well. We should probably be consistent in using these tools.

The souffle goal doesn’t have a Ports check because there’s not Portsfile available for Souffle.

@behnazh-w
Copy link
Member

Lastly, I think this line

echo "Unable to install Souffle. Please install it manually." && exit 0; \

@jenstroeger Thanks. That's actually intentional. We just want to let the user know that they should install Souffle manually, but the setup should not be considered as failed.

…on macOS gracefully

Signed-off-by: Jens Troeger <jens.troeger@light-speed.de>
@behnazh-w behnazh-w merged commit 6ef2447 into oracle:staging Sep 30, 2024
9 checks passed
art1f1c3R pushed a commit that referenced this pull request Nov 29, 2024
…on macOS gracefully (#877)

Signed-off-by: Jens Troeger <jens.troeger@light-speed.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants