Skip to content

feat: get_cwd and set_cwd #1014

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

feat: get_cwd and set_cwd #1014

wants to merge 10 commits into from

Conversation

wassup05
Copy link
Contributor

@wassup05 wassup05 commented Jul 5, 2025

Depends on #1011

  1. User facing functions added are
  • get_cwd (cwd, err) - Gets the current working directory (cwd) of the process.
  • set_cwd (path, err) - Sets the current working directory (cwd) of the process.
  1. Binds to getcwd, chdir on Unix and _getcwd, _chdir on Windows
  2. Error handling is done through state_type and both the platform specific error code and the user friendly error message are provided if an error occurs

@wassup05
Copy link
Contributor Author

wassup05 commented Jul 7, 2025

I am honestly not sure how I should go about testing these functions independently

@sebastian-mutz
Copy link
Member

I am honestly not sure how I should go about testing these functions independently

If I uncerstand correctly, it looks like you're already doing that by invoking execute_command_line; that seems like the best solution to me, but perhaps others think of something else..

@wassup05
Copy link
Contributor Author

wassup05 commented Jul 9, 2025

Those are commits from the other branch @sebastian-mutz, the one in this PR: #1014

I don't think I can use execute_command_line here, because that spawns a child process to run the command and using cd would change the working directory of that child process not the parent one... But maybe there is some workaround I am not aware of

@sebastian-mutz sebastian-mutz requested a review from perazz July 21, 2025 15:54
@sebastian-mutz
Copy link
Member

sebastian-mutz commented Jul 22, 2025

Suggestions for testing (summarising from the chat with Jose):

For testing get_cwd:
gfortran: GETCWD
Intel : GETCWD
Flang: GETCWD

For testing set_cwd:
gfortran: CHDIR
Intel: CHDIR
Flang: CHDIR

Alternatively, using execute_command_line should be fine.

Copy link
Member

@perazz perazz left a comment

Choose a reason for hiding this comment

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

Thank you for this PR @wassup05. Here is a couple of comments:

  • the state handler err should be optional. So, the internal handleing features returning the state variable if present, or triggering an error stop if not. This will make behavior consistent to everywhere else.
  • For testing: I think set/get can be tested in the same test program imho. Here is an example:
  1. get current directory and save it to "pwd" variable -> must be successful
  2. create a new local temporary directory -> must be successful
  3. set_cwd to that
  4. get_cwd should now return the temporary directory
  5. set_cwd back to "pwd"
  6. check successful

I suggest to merge this PR after directory handling is merged into the library already.

Copy link
Member

@perazz perazz left a comment

Choose a reason for hiding this comment

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

Here are a few additional comments. I suggest the err output variable to be made optional in all cases, consistent with the library. call err0%handle(err) already includes the option to halt on error, if err is not present.

@wassup05 wassup05 requested a review from perazz August 1, 2025 10:29
Copy link
Member

@perazz perazz left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @wassup05. As a follow up, may I suggest that the C->Fortran string conversion routine be put, part of a separate PR, into the stdlib_strings module into the to_string interface.

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