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 PWD environment entry if cwd provided to Runner() or Runner.run() is a Path-object #7107

Merged
merged 5 commits into from
Oct 24, 2022

Conversation

christian-monch
Copy link
Contributor

Fixes #7040

This PR fixes #7040. It ensures that an environment that is provided to the subprocess contains a proper entry for PWD. That means, the entry contains a string that describes the path of the working directory, and not a python-representation of a Path object.

@christian-monch christian-monch changed the title Fix $PWD environment entry if cwd' provided to Runner() or Runner.run() is a Path`-object Fix PWD environment entry if cwd provided to Runner() or Runner.run() is a Path-object Oct 24, 2022
@christian-monch christian-monch changed the base branch from master to maint October 24, 2022 08:54
@codeclimate
Copy link

codeclimate bot commented Oct 24, 2022

Code Climate has analyzed commit 0d727bb and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3

View more on Code Climate.

@christian-monch christian-monch force-pushed the issue-7040-runner-cwd-path branch from 0d727bb to 0268ed4 Compare October 24, 2022 08:58
@christian-monch christian-monch added the semver-internal Changes only affect the internal API label Oct 24, 2022
bpoldrack
bpoldrack previously approved these changes Oct 24, 2022
Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

Thx, @christian-monch. LGTM.

I have two suggestions, though.



@with_tempfile(mkdir=True)
def test_environment(temp_dir_path=None):
Copy link
Member

Choose a reason for hiding this comment

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

This test makes me think we have not covered the case of passing cwd to the constructor and (a different one) to the method. We probably should test that behavior, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are tested. Line 336 -337 test passing to method, line 341-342 test passing to the constructor. Or did I missunderstand you?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you did. We do not test what happens if the caller sets (possibly different) cwd via both - constructor and method. This is only testing the case where the caller does either one of those.

Copy link
Member

Choose a reason for hiding this comment

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

In code: Runner(cwd="one/path").run(cwd="another/path") - what happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In code: Runner(cwd="one/path").run(cwd="another/path") - what happens?

The method provided variables take precedence over the constructor provided. Don't ask me what the original authors intended with this behavior, it might be suited to reusing "pre-configured" runners multiple times with minimal source code

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I hope, that's what happens. Point was, that this behavior seems untested.
Anyways - there was no test before, so I'm still happy to merge as is.

changelog.d/pr-7107.md Outdated Show resolved Hide resolved
Swap outer string delimiters from `"´ to `'´, to satisfy certain
windows environments.

Put `SYSTEMROOT` into the environment passed to subprocesses on
windows in order to fix errors like: cannot initialize random
number generator (some libs can not be found).
@christian-monch christian-monch force-pushed the issue-7040-runner-cwd-path branch from a988523 to 4c1413b Compare October 24, 2022 10:16
@bpoldrack bpoldrack dismissed their stale review October 24, 2022 11:43

Failed to catch the windows error

@bpoldrack
Copy link
Member

One more thing, @christian-monch: Looks like the command quoting in the test (py2cmd( ...) is wrong for windows. We do have a helper for that somewhere (trying to find).

@christian-monch
Copy link
Contributor Author

christian-monch commented Oct 24, 2022

One more thing, @christian-monch: Looks like the command quoting in the test (py2cmd( ...) is wrong for windows. We do have a helper for that somewhere (trying to find).

py2cmd is the helper. I copied it from an older version of datalad.tests.test_witless_runner.py, and just added the additional argumnets, which are not used in this context. It works on some windows environments, and not on others. I think the reason might be in the selection of string-delimiters within the python code-string. It seems that Popen on some windows environments uses " to delimit the arguments when they are passed down to the system. That conflicts with using them in the python code (the error message does not reflect the string that is passed down, but a garbled version. The passed down string only contains one double-quote around PWD, i.e. ["PWD"]).

Added:
I pushed an update that uses ' within the python code string. That should fix the issue on CI.

I will not change py2cmd before we figure out what exactly is going on on Windows.

@bpoldrack
Copy link
Member

I pushed an update that uses ' within the python code string. That should fix the issue on CI.
I will not change py2cmd before we figure out what exactly is going on on Windows.

Agreed. Thx!

@christian-monch christian-monch force-pushed the issue-7040-runner-cwd-path branch from 69c2148 to 7beffc1 Compare October 24, 2022 13:40
@christian-monch
Copy link
Contributor Author

Amended the PR with a test regarding the parameter priorities.

@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Base: 88.34% // Head: 88.90% // Increases project coverage by +0.55% 🎉

Coverage data is based on head (7beffc1) compared to base (239d9b2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7107      +/-   ##
==========================================
+ Coverage   88.34%   88.90%   +0.55%     
==========================================
  Files         355      355              
  Lines       46417    46533     +116     
==========================================
+ Hits        41007    41369     +362     
+ Misses       5410     5164     -246     
Impacted Files Coverage Δ
datalad/runner/runner.py 100.00% <100.00%> (ø)
datalad/runner/tests/test_witless_runner.py 96.29% <100.00%> (+0.90%) ⬆️
datalad/customremotes/tests/test_datalad.py 95.34% <0.00%> (-2.28%) ⬇️
datalad/distribution/create_sibling.py 65.24% <0.00%> (-0.19%) ⬇️
datalad/cli/common_args.py 100.00% <0.00%> (ø)
datalad/tests/test_dochelpers.py 100.00% <0.00%> (ø)
datalad/support/tests/test_network.py 100.00% <0.00%> (ø)
datalad/distribution/tests/test_get.py 100.00% <0.00%> (ø)
datalad/customremotes/tests/test_archives.py 89.36% <0.00%> (+0.07%) ⬆️
datalad/support/gitrepo.py 91.02% <0.00%> (+0.07%) ⬆️
... and 52 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bpoldrack
Copy link
Member

Thanks, @christian-monch!

@bpoldrack bpoldrack merged commit 969bb82 into datalad:maint Oct 24, 2022
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.17.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Create a release when this pr is merged semver-internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runner cannot handle run(cwd=Path)
4 participants