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

Fixes bash command error when DESTDIR is a non-path directory #431

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

frshwtr
Copy link

@frshwtr frshwtr commented Jun 12, 2020

Here are some helpful tips you can follow when submitting a pull request:

  1. Fork the repository and create your branch from master.
  2. Run make build in the repository root.
  3. If you've fixed a bug or added code that should be tested, add tests!
  4. Ensure the test suite passes (make test).
  5. The --debug flag is often helpful for debugging HTTP client requests and responses.
  6. Format your code with gofmt.
  7. Make sure your code lints (make lint). Note: This requires Docker to run inside a local job.

If you have any questions, feel free to ping us at @CircleCI-Public/x-team.

Problem

We found an issue with the install script on MacOS Catalina with bash 3.2 and Ubuntu 18 with bash 4.4, zsh 5.4.2.

When DESTDIR is set to a directory that is not in PATH, CircleCI CLI will successfully install but the script will error out on line 59.

Example output:

➜ DESTDIR=$(pwd)/clitemp ./install.sh
Starting installation.
Installing CircleCI CLI v0.1.7971
Installing to /home/ben/git/frshwtr/circleci-cli/clitemp
An error occured installing the tool.
The contents of the directory /tmp/tmp.HAAnGm47op have been left in place to help to debug the issue.

We suspect this is because command is being executed with no directory context. Meaning any install to a non-path directory will fail, causing the install script to fail.

Testing

We have ran this locally on macOS Catalina with the following cases:

  • settingDESTDIR and examining the exit code and status of the installed binary.
  • doing the same with the default (/usr/local/bin) set by the script.

All appears to work well in both cases.

@frshwtr frshwtr requested a review from a team as a code owner June 12, 2020 16:14
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #431 (4d8be28) into master (168582f) will decrease coverage by 1.91%.
The diff coverage is n/a.

❗ Current head 4d8be28 differs from pull request most recent head 330d65b. Consider uploading reports for the commit 330d65b to get more accurate results

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
- Coverage   32.77%   30.86%   -1.92%     
==========================================
  Files          46       26      -20     
  Lines        5415     3201    -2214     
==========================================
- Hits         1775      988     -787     
+ Misses       3387     2119    -1268     
+ Partials      253       94     -159     
Impacted Files Coverage Δ
filetree/filetree.go 73.72% <0.00%> (-4.54%) ⬇️
cmd/context.go 25.14% <0.00%> (-3.05%) ⬇️
references/references.go 60.60% <0.00%> (-3.04%) ⬇️
git/git.go 81.25% <0.00%> (-2.36%) ⬇️
cmd/update.go 23.76% <0.00%> (-0.38%) ⬇️
cmd/diagnostic.go 17.30% <0.00%> (-0.20%) ⬇️
main.go 0.00% <0.00%> (ø)
cmd/open.go 50.00% <0.00%> (ø)
cmd/check.go 0.00% <0.00%> (ø)
cmd/disabled.go 0.00% <0.00%> (ø)
... and 37 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@jvincent42 jvincent42 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Sorry for the delay in reviewing your work.
If you still want this PR to be merged, can you please rebase it on main and address the comment I've left ?
Thank you for your time.

@@ -56,7 +56,7 @@ curl -sL --retry 3 "${RELEASE_URL}" | tar zx --strip 1
echo "Installing to $DESTDIR"
install circleci "$DESTDIR"

command -v circleci
command -v "$DESTDIR"circleci
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to remove any potential trailing / in DESTDIR and force a / before the binary name.

Suggested change
command -v "$DESTDIR"circleci
command -v "${DESTDIR%/}"/circleci

@jvincent42 jvincent42 self-assigned this Oct 13, 2023
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