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

BF: Fix PWD to match provided cwd in Runner #3215

Merged
merged 3 commits into from Mar 12, 2019

Conversation

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Mar 11, 2019

We do take care about tuning up PWD while chdir etc. But when we run
external process and set its cwd, we left PWD unchanged if was provided
in (likely) inherited and tuned environment. This fix would change
PWD to match provided cwd, if it is known to the environment and cwd
is set.

I was not bold enough to always get an env (if not provided) and change it
there, but may be that is what we will need to end up doing at some
point.

This should resolve the failing test in
#3184
caused by underlying datalad process, which is started from a temp directory
while we provide it with a now non-matching PWD:
https://travis-ci.org/datalad/datalad/jobs/501525590#L4325

We do take care about tuning up PWD while chdir etc.  But when we run
external process and set its cwd, we left PWD unchanged if was provided
in (likely) inherited and tuned environment.  This fix would change
PWD to match provided cwd, if it is known to the environment and cwd
is set.

I was not bold enough to always get an env (if not provided) and change it
there, but may be that is what we will need to end up doing at some
point.

This should resolve the failing test in
datalad#3184
caused by underlying datalad process, which is started from a temp directory
while we provide it with a now non-matching PWD:
https://travis-ci.org/datalad/datalad/jobs/501525590#L4325
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Mar 11, 2019

it got me thinking -- may be we should check and reset PWD upon start of datalad right away but then without a warning

@codecov
Copy link

@codecov codecov bot commented Mar 11, 2019

Codecov Report

Merging #3215 into 0.11.x will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.11.x    #3215      +/-   ##
==========================================
- Coverage   90.88%   90.86%   -0.02%     
==========================================
  Files         249      249              
  Lines       32845    32858      +13     
==========================================
+ Hits        29850    29856       +6     
- Misses       2995     3002       +7
Impacted Files Coverage Δ
datalad/cmd.py 95.71% <100%> (+0.06%) ⬆️
datalad/tests/test_cmd.py 97.56% <100%> (+0.14%) ⬆️
datalad/downloaders/http.py 82.53% <0%> (-2.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e2d1ab...b1ac524. Read the comment docs.

@@ -285,6 +285,18 @@ def test_runner_failure_unicode(path):
runner.run(u"β-command-doesnt-exist", cwd=path)


@skip_if_on_windows # likely would fail
@with_tempfile(mkdir=True)
def test_runner_fix_PWD(path):
Copy link
Contributor

@kyleam kyleam Mar 11, 2019

This test passes when I apply it, without the cmd.py changes, on top of this PR's base.

% git rev-parse HEAD
5d7014599a1476355df986d167f4b14689c1ff0c

% python -m nose -vs datalad/tests/test_cmd.py:test_runner_fix_PWD
datalad.tests.test_cmd.test_runner_fix_PWD ... ok
Versions: appdirs=1.4.3 boto=2.49.0 cmd:annex=7.20190219 cmd:git=2.21.0 cmd:system-git=2.21.0 cmd:system-ssh=7.4p1 exifread=2.1.2 git=2.1.11 gitdb=2.0.5 humanize=0.5.1 iso8601=0.1.12 keyring=18.0.0 keyrings.alt=3.1.1 msgpack=0.6.1 mutagen=1.42.0 requests=2.21.0 six=1.12.0 wrapt=1.11.1
Obscure filename: str=b' "\';a&b&c\xce\x94\xd0\x99\xd7\xa7\xd9\x85\xe0\xb9\x97\xe3\x81\x82 `| ' repr=' "\';a&b&cΔЙקم๗あ `| '
Encodings: default='utf-8' filesystem='utf-8' locale.prefered='UTF-8'
Environment: LANG='en_US.UTF-8' PYTHONPATH='/home/kyle/.local/lib/python' GIT_SSL_CAINFO='/home/kyle/.guix-profile/etc/ssl/certs/ca-certificates.crt' GIT_EXEC_PATH='/home/kyle/.guix-profile/libexec/git-core' PATH='/home/kyle/src/python/venvs/datalad/bin:/home/kyle/src/python/venvs/datalad//bin:/home/kyle/.guix-profile/bin:/home/kyle/.config/guix/current/bin:/home/kyle/.cabal/bin:/usr/lib/ccache/bin:/home/kyle/.local/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games'

----------------------------------------------------------------------
Ran 1 test in 0.091s

OK

Copy link
Member Author

@yarikoptic yarikoptic Mar 11, 2019

strange -- will check

Copy link
Member Author

@yarikoptic yarikoptic Mar 11, 2019

ha -- it is (99% confidence guess) because test uses "shell=True" so sure thing that shell fixes it up. So the fixup is needed only if shell is not True. Thanks for catching this!!!

@yarikoptic yarikoptic changed the base branch from master to 0.11.x Mar 11, 2019
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Mar 11, 2019

changed base to 0.11.x

env['PWD'] = orig_cwd = os.getcwd()
runner = Runner(cwd=path, env=env)
out, err = runner.run(
['python', '-c', 'import os; print(os.environ["PWD"])'],
Copy link
Contributor

@kyleam kyleam Mar 11, 2019

Elsewhere we've avoided assuming python is available (see #2287). How about using sys.executable instead?

Copy link
Member Author

@yarikoptic yarikoptic Mar 11, 2019

right! that was the intention - will fix up now, thanks!

@yarikoptic yarikoptic force-pushed the bf-fix-PWD-in-run branch from 2f32472 to b1ac524 Mar 11, 2019
kyleam
kyleam approved these changes Mar 11, 2019
Copy link
Contributor

@kyleam kyleam left a comment

Thanks for the updates.

@yarikoptic yarikoptic merged commit d046e2b into datalad:0.11.x Mar 12, 2019
5 checks passed
@yarikoptic yarikoptic added this to the Release 0.11.4 milestone Mar 14, 2019
yarikoptic added a commit that referenced this issue Apr 6, 2019
    ## 0.11.4 (Mar 18, 2019) -- get-ready

    Largely a bug fix release with a few enhancements

    ### Important

    - 0.11.x series will be the last one with support for direct mode of [git-annex][]
      which is used on crippled (no symlinks and no locking) filesystems.
      v7 repositories should be used instead.

    ### Fixes

    - Extraction of .gz files is broken without p7zip installed.  We now
      abort with an informative error in this situation.  ([#3176][])

    - Committing failed in some cases because we didn't ensure that the
      path passed to `git read-tree --index-output=...` resided on the
      same filesystem as the repository.  ([#3181][])

    - Some pointless warnings during metadata aggregation have been
      eliminated.  ([#3186][])

    - With Python 3 the LORIS token authenticator did not properly decode
      a response ([#3205][]).

    - With Python 3 downloaders unnecessarily decoded the response when
      getting the status, leading to an encoding error.  ([#3210][])

    - In some cases, our internal command Runner did not adjust the
      environment's `PWD` to match the current working directory specified
      with the `cwd` parameter.  ([#3215][])

    - The specification of the pyliblzma dependency was broken.  ([#3220][])

    - [search] displayed an uninformative blank log message in some
      cases.  ([#3222][])

    - The logic for finding the location of the aggregate metadata DB
      anchored the search path incorrectly, leading to a spurious warning.
      ([#3241][])

    - Some progress bars were still displayed when stdout and stderr were
      not attached to a tty.  ([#3281][])

    - Check for stdin/out/err to not be closed before checking for `.isatty`.
      ([#3268][])

    ### Enhancements and new features

    - Creating a new repository now aborts if any of the files in the
      directory are tracked by a repository in a parent directory.
      ([#3211][])

    - [run] learned to replace the `{tmpdir}` placeholder in commands with
      a temporary directory.  ([#3223][])

    - [duecredit][] support has been added for citing DataLad itself as
      well as datasets that an analysis uses.  ([#3184][])

    - The `eval_results` interface helper unintentionally modified one of
      its arguments.  ([#3249][])

    - A few DataLad constants have been added, changed, or renamed ([#3250][]):
      - `HANDLE_META_DIR` is now `DATALAD_DOTDIR`.  The old name should be
         considered deprecated.
      - `METADATA_DIR` now refers to `DATALAD_DOTDIR/metadata` rather than
        `DATALAD_DOTDIR/meta` (which is still available as
        `OLDMETADATA_DIR`).
      - The new `DATASET_METADATA_FILE` refers to `METADATA_DIR/dataset.json`.
      - The new `DATASET_CONFIG_FILE` refers to `DATALAD_DOTDIR/config`.
      - `METADATA_FILENAME` has been renamed to `OLDMETADATA_FILENAME`.

* tag '0.11.4': (82 commits)
  Updated CHANGELOG.md for having merged check for not being closed
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.4 entries
  CHANGELOG.md: Update 0.11.4 entries
  CHANGELOG.md: Adjust the format of a link
  BF: split lines on spaces and commas befoe doing sed capture of #issue
  RF: adjust tools/link_issues_CHANGELOG to have MD links as [#issue][]
  BF+DOC: Fix all links to mark them valid markdown
  BF: Fix markup bug that prevents sphinx-build from succeeding
  DOC: extend coumentation of is_interactive
  BF: guard .istty with try/except
  ENH: ui: Drop progress bars if not attached to tty
  ENH: ui: Add another name for SilentConsoleLog
  BF: check for stdin/out/err to not be closed before checking for .isatty
  RF: HANDLE_META_DIR -> DATALAD_DOTDIR (but keeping an alias for compatibility)
  RF: define/use consts DATASET_CONFIG_FILE, DATASET_METADATA_FILE, METADATA_DIR
  RF: METADATA_DIR/FILE -> OLDMETADATA_DIR/FILE
  BF: do not cause a side-effect on kwargs in @eval_results
  BF: join with ds.path when trying to see if any other metadata file is available
  RF: Move duecredit into its own section so it is not a part of install_requires
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.4 entries
  ...
yarikoptic added a commit to yarikoptic/datalad that referenced this issue Apr 6, 2019
    ## 0.11.4 (Mar 18, 2019) -- get-ready

    Largely a bug fix release with a few enhancements

    ### Important

    - 0.11.x series will be the last one with support for direct mode of [git-annex][]
      which is used on crippled (no symlinks and no locking) filesystems.
      v7 repositories should be used instead.

    ### Fixes

    - Extraction of .gz files is broken without p7zip installed.  We now
      abort with an informative error in this situation.  ([datalad#3176][])

    - Committing failed in some cases because we didn't ensure that the
      path passed to `git read-tree --index-output=...` resided on the
      same filesystem as the repository.  ([datalad#3181][])

    - Some pointless warnings during metadata aggregation have been
      eliminated.  ([datalad#3186][])

    - With Python 3 the LORIS token authenticator did not properly decode
      a response ([datalad#3205][]).

    - With Python 3 downloaders unnecessarily decoded the response when
      getting the status, leading to an encoding error.  ([datalad#3210][])

    - In some cases, our internal command Runner did not adjust the
      environment's `PWD` to match the current working directory specified
      with the `cwd` parameter.  ([datalad#3215][])

    - The specification of the pyliblzma dependency was broken.  ([datalad#3220][])

    - [search] displayed an uninformative blank log message in some
      cases.  ([datalad#3222][])

    - The logic for finding the location of the aggregate metadata DB
      anchored the search path incorrectly, leading to a spurious warning.
      ([datalad#3241][])

    - Some progress bars were still displayed when stdout and stderr were
      not attached to a tty.  ([datalad#3281][])

    - Check for stdin/out/err to not be closed before checking for `.isatty`.
      ([datalad#3268][])

    ### Enhancements and new features

    - Creating a new repository now aborts if any of the files in the
      directory are tracked by a repository in a parent directory.
      ([datalad#3211][])

    - [run] learned to replace the `{tmpdir}` placeholder in commands with
      a temporary directory.  ([datalad#3223][])

    - [duecredit][] support has been added for citing DataLad itself as
      well as datasets that an analysis uses.  ([datalad#3184][])

    - The `eval_results` interface helper unintentionally modified one of
      its arguments.  ([datalad#3249][])

    - A few DataLad constants have been added, changed, or renamed ([datalad#3250][]):
      - `HANDLE_META_DIR` is now `DATALAD_DOTDIR`.  The old name should be
         considered deprecated.
      - `METADATA_DIR` now refers to `DATALAD_DOTDIR/metadata` rather than
        `DATALAD_DOTDIR/meta` (which is still available as
        `OLDMETADATA_DIR`).
      - The new `DATASET_METADATA_FILE` refers to `METADATA_DIR/dataset.json`.
      - The new `DATASET_CONFIG_FILE` refers to `DATALAD_DOTDIR/config`.
      - `METADATA_FILENAME` has been renamed to `OLDMETADATA_FILENAME`.

* tag '0.11.4':
  Updated CHANGELOG.md for having merged check for not being closed
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.4 entries
  CHANGELOG.md: Update 0.11.4 entries
  CHANGELOG.md: Adjust the format of a link
  BF: split lines on spaces and commas befoe doing sed capture of #issue
  RF: adjust tools/link_issues_CHANGELOG to have MD links as [#issue][]
  BF+DOC: Fix all links to mark them valid markdown
  BF: Fix markup bug that prevents sphinx-build from succeeding
  DOC: extend coumentation of is_interactive
  BF: guard .istty with try/except
  BF: check for stdin/out/err to not be closed before checking for .isatty
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.4 entries
  CHANGELOG: Add entries for 0.11.4
  ENH: version boost and initial changes to CHANGELOG
@yarikoptic yarikoptic deleted the bf-fix-PWD-in-run branch Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants