Skip to content

Zbeekman mac os patch - #6

Closed
zbeekman wants to merge 2 commits into
pocc:masterfrom
zbeekman:zbeekman-macOS-patch
Closed

Zbeekman mac os patch#6
zbeekman wants to merge 2 commits into
pocc:masterfrom
zbeekman:zbeekman-macOS-patch

Conversation

@zbeekman

@zbeekman zbeekman commented Dec 4, 2019

Copy link
Copy Markdown

I can squash down the commits, etc.

Should help fix issues raised in #4

@zbeekman

zbeekman commented Dec 4, 2019

Copy link
Copy Markdown
Author

One, moment, this doesn't work...

@pocc

pocc commented Dec 4, 2019

Copy link
Copy Markdown
Owner

Will merge once it does.

@zbeekman

zbeekman commented Dec 4, 2019

Copy link
Copy Markdown
Author

@pocc: AFAICT, this is now ready to merge. Here are a few notes:

  1. Fix for non-robust "$0" directory finding was adapted from both:
    1. https://www.ostricher.com/2014/10/the-right-way-to-get-the-directory-of-a-bash-script/ ("Final attempt: Resolve symlinks in $BASH_SOURCE")
    2. http://stackoverflow.com/questions/59895/can-a-bash-script-tell-what-directory-its-stored-in
  2. clang-tidy on my system was producing a new, unexpected error for err.cpp
    1. c055e3e modifies the test to instruct clang-tidy to ignore that particular error, but a more robust solution would be to make the tests less tightly coupled to the output
  3. oclint on my system has a patch revision (0.13.1, NOT 0.13) which breaks the test. I did not fix this.
  4. oclint seems to output errors/warnings in a non-deterministic order, and the test breaks when the order changes. I did not fix this either.

Also, on macOS, the homebrew cask for oclint conflicts with GCC (won't install, and causes issues if it is installed and you try to install/link gcc). A fix for this was just merged: Homebrew/homebrew-cask#73649

@zbeekman

zbeekman commented Dec 4, 2019

Copy link
Copy Markdown
Author

@pocc if you want me to drop the test modification (if you have plans for a better fix) or want me to make any changes, please let me know.

@guihao-liang guihao-liang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it better to keep this function in one place instead of several copies?

Comment thread hooks/clang-format
SOURCE="${BASH_SOURCE[0]}"
# While $SOURCE is a symlink, resolve it
while [ -h "$SOURCE" ]; do
DIR="$( cd -P "$( dirname "$SOURCE" )" > /dev/null && pwd )"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's no need to keep extra space around parenthesis.

Comment thread hooks/clang-format
args=("$@")
hook_dir="$(dirname "$(readlink -f "$0")")"
get_script_dir () {
SOURCE="${BASH_SOURCE[0]}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$0 is sufficient.

Comment thread hooks/clang-format
args=("$@")
hook_dir="$(dirname "$(readlink -f "$0")")"
get_script_dir () {
SOURCE="${BASH_SOURCE[0]}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you may consider making it local, local source_dir=

@guihao-liang

Copy link
Copy Markdown
Contributor

Sorry, I didn't mean to be a reviewer.

@guihao-liang

guihao-liang commented Jan 26, 2020

Copy link
Copy Markdown
Contributor

I found this link pretty helpful to have the right readlink workaround.

Besides that, it's better to use $OSTYPE to detect os first, if it's "linux-gnu", use readlink -f directly.

@guihao-liang

Copy link
Copy Markdown
Contributor

eep this function in one place instead of several copies?

nvm, it's a chicken and egg problem.

@pocc
pocc force-pushed the master branch 2 times, most recently from 4094253 to d0b7ef2 Compare March 21, 2020 22:57
@pocc

pocc commented Mar 23, 2020

Copy link
Copy Markdown
Owner

Closing as this no longer necessary. All of the utils functions have been merged into each respective file for this bash version and a python version is now available. @zbeekman thank you for making this PR.

If I'm missing something, please reply here.

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.

3 participants