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(cli): properly detect XSPEC_HOME #1568

Merged
merged 3 commits into from
Nov 19, 2021

Conversation

mcwayliffe
Copy link

Using $(dirname "$0") is not a safe way to get the location of a running script.

For example, if you run:

cd ~/xspec/bin
./xspec.sh -h

the value of XSPEC_HOME will be .. Not what you want. More reliable to cd && pwd

@AirQuick AirQuick added the bug label Nov 17, 2021
@AirQuick AirQuick added this to the v2.3 milestone Nov 17, 2021
@AirQuick AirQuick changed the title fix(cli): Properly detect XSPEC_HOME fix(cli): properly detect XSPEC_HOME Nov 17, 2021
@AirQuick
Copy link
Member

Thanks for finding and fixing the issue.
Can you refine the line? shellcheck is complaining about missing quotes: https://github.com/xspec/xspec/runs/4239689718?check_suite_focus=true#step:5:15

@AirQuick
Copy link
Member

@mcwayliffe
Thanks for the new commit.

Now I added a regression test 72d0105 to your branch. Does it look good to you? If so (or if you don't care about the test), I'll merge this pull request.

@mcwayliffe
Copy link
Author

@AirQuick LGTM 👍

Copy link
Member

@AirQuick AirQuick left a comment

Choose a reason for hiding this comment

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

Though I'm not very familiar with shell scripts, the fix looks good according to https://unix.stackexchange.com/a/118438.
I'm merging this (after integrating the current master in order to fulfill the required statuses).

@AirQuick AirQuick enabled auto-merge (squash) November 19, 2021 02:37
@AirQuick AirQuick merged commit 2dbdfd1 into xspec:master Nov 19, 2021
@mcwayliffe mcwayliffe deleted the xspec-home-detection branch February 25, 2022 15:19
@AirQuick AirQuick mentioned this pull request Aug 27, 2023
16 tasks
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.

None yet

2 participants