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

Update activate #392

Merged
merged 5 commits into from
Apr 27, 2023
Merged

Update activate #392

merged 5 commits into from
Apr 27, 2023

Conversation

trvon
Copy link
Contributor

@trvon trvon commented Apr 19, 2023

Moved "export KCTF_CTF_DIR" into macOS check to use grealpath instead of realpath which doesn't seem supported by default.

Moved "export KCTF_CTF_DIR" into macOS check to use grealpath instead of realpath which doesn't seem supported.
@google-cla
Copy link

google-cla bot commented Apr 19, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

trvon added 2 commits April 19, 2023 09:56
Moved "export KCTF_CTF_DIR" into macOS check to use grealpath instead of realpath which doesn't seem supported.
@sroettger
Copy link
Collaborator

Hi, thanks for the pull request!
On my mac I have the opposite problem, realpath is available but grealpath is not..

Could you make it check which one exists and then put that in an environment variable maybe?

@trvon
Copy link
Contributor Author

trvon commented Apr 20, 2023

Real path does exist on my system. I should correct my previous statement to be that how realpath is used, does not seem to be have the intended behavior. In my environment, when I source the kctf/activate after a clean download I see:

realpath: illegal option -- -
usage: realpath [-q] [path ...]
kctf/activate:source:50: no such file or directory: /kctf/bin/kctf-log
gstat: cannot stat '/kctf/bin/kctf-cluster': No such file or directory
_kctf_check_umask:6: command not found: _kctf_log_err
_kctf_error_cleanup:unset:12: no such hash table element: _kctf_log
_kctf_error_cleanup:unset:13: no such hash table element: _kctf_log_err

I can update my commit to include the follow so it will work in both environments.

  if ! grealpath &> /dev/null
  then
    export KCTF_CTF_DIR="$(grealpath --no-symlinks "$(dirname "${BASH_SOURCE-$0}")/..")"
  else
    export KCTF_CTF_DIR="$(realpath --no-symlinks "$(dirname "${BASH_SOURCE-$0}")/..")"
  fi

@sroettger
Copy link
Collaborator

sroettger commented Apr 21, 2023

ah, that makes sense.
I think the better solution would be to skip the --no-symlinks in that case. I think it's there to support a symlinked kctf directory inside the challenge dir.

I.e. maybe something like this should work?

export KCTF_CTF_DIR="$(realpath "$(dirname "$(dirname "${BASH_SOURCE-$0}")")")"

In a quick test, that seems to handle all the cases of relative and absolute paths with a symlinked kctf dir

... Thinking more about it, it needs some handling of source ./activate. So maybe more like:

script_dir="$(dirname "${BASH_SOURCE-$0}")"
if [[ "$script_dir" == "." ]]; then
  script_dir="../."
fi
export KCTF_CTF_DIR="$(realpath "$(dirname "${script_dir}")")"
unset script_dir

@trvon
Copy link
Contributor Author

trvon commented Apr 21, 2023

That solution worked as well in my initial testing. I'll update my branch to reflect the suggestion.

@sroettger
Copy link
Collaborator

That solution worked as well in my initial testing. I'll update my branch to reflect the suggestion.

Sounds good. Please let me know once you updated it.

@trvon
Copy link
Contributor Author

trvon commented Apr 26, 2023

Just updated the code.

@sroettger sroettger merged commit 7ba35c4 into google:v1 Apr 27, 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.

2 participants